Shot history account for post brewing drip #357
Conversation
|
Warning Rate limit exceeded@dragm83 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (3)
WalkthroughAdds an extendedRecording lifecycle with weight-stabilization and hard timeout, defers history file closure until both recording modes end, extracts finalization into finalizeRecording(), increases MAX_HISTORY_ENTRIES and timing constants, and tightens web CSV parsing to derive volume from max-valid samples with per-line validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant P as ShotHistoryPlugin
participant S as Scale/Bluetooth
participant V as VolumetricSensor
participant FS as FileStore
U->>P: startRecording()
P->>P: reset timers, lastStableWeight, open history file
loop Recording
S-->>P: weight sample
P->>P: validate & update samples
alt shouldRecord (MODE_BREW && (recording || extendedRecording))
P->>FS: write sample
end
V-->>P: optional volumetric data
end
U->>P: endRecording()
alt volumetric available && currentBluetoothWeight > 0
P->>P: start extendedRecording (init timers, lastStableWeight)
loop monitor stability / timeout
S-->>P: weight sample
P->>P: compute weightDiff = abs(current - lastStableWeight)
alt weightDiff < THRESHOLD for WEIGHT_STABILIZATION_TIME
P->>P: conclude extendedRecording -> finalizeRecording()
else if elapsed > EXTENDED_RECORDING_DURATION
P->>P: conclude extendedRecording -> finalizeRecording()
else
P->>P: update lastStableWeight / reset timers
end
P->>FS: continue writing while extendedRecording
end
else
P->>P: finalizeRecording()
end
P->>P: finalizeRecording()
alt duration <= 7500 ms
P->>FS: delete .dat (and .json notes if present)
else
P->>P: increment history index & cleanupHistory()
end
note right of P: Close history file only when !recording && !extendedRecording
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (4)
web/src/pages/ShotHistory/utils.js (1)
31-35: Guard against empty samples to avoid runtime errorsWhen samples is empty, lastSample becomes undefined and accessing lastSample.t/v will throw.
- if (data['samples']) { - const lastSample = data['samples'][data['samples'].length - 1]; - data.duration = lastSample.t; - data.volume = lastSample.v; - } + if (data['samples'] && data['samples'].length > 0) { + const lastSample = data['samples'][data['samples'].length - 1]; + data.duration = lastSample.t; + data.volume = lastSample.v; + }src/display/plugins/ShotHistoryPlugin.h (1)
29-46: Wrong printf format specifier fort(unsigned long)
tis an unsigned long but you use%din serialize(). Use%luto avoid UB/truncation.Apply this diff:
- std::string serialize() { - return string_format("%d,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f", t, tt, ct, tp, cp, fl, tf, pf, vf, v, - ev); - } + std::string serialize() { + return string_format("%lu,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f", + t, tt, ct, tp, cp, fl, tf, pf, vf, v, ev); + }src/display/plugins/ShotHistoryPlugin.cpp (2)
51-54: Guard header write withisFileOpento avoid writing to an invalid FileIf opening fails, header write attempts on an unopened File.
Apply this diff:
- if (!headerWritten) { + if (!headerWritten && isFileOpen) { file.printf("1,%s,%ld\n", currentProfileName.c_str(), getTime()); headerWritten = true; }
212-233: ArduinoJson misuse: constructingJsonDocumentand copying; deserialize directly into response
JsonDocument notes;is invalid (cannot instantiate). Also, copying documents is wasteful. Deserialize notes directly into the response object’s memory.Apply this diff:
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - o["notes"] = notes; - } + // Also include notes if they exist + if (!o.containsKey("notes")) { + if (!loadNotes(id, o["notes"])) { + o.remove("notes"); + } + }Note: This depends on the header/cpp refactor of loadNotes to accept a JsonVariant destination.
🧹 Nitpick comments (9)
src/display/plugins/BLEScalePlugin.cpp (1)
15-15: Include added: ensure header file exists across all build variantsGood to add the header, but confirm it exists for all targets/configs to avoid build breaks in variants where this plugin isn’t compiled.
Use the script above to verify header presence and reference usage.
web/src/pages/ShotHistory/utils.js (1)
37-39: Avoid mutating the original shot.notes by returning a copyAssigning the same object reference may cause subtle UI state coupling if the returned data is mutated downstream. Spread to decouple.
- if (shot.notes) { - data.notes = shot.notes; - } + if (shot.notes) { + data.notes = typeof shot.notes === 'object' ? { ...shot.notes } : shot.notes; + }schema/shot_notes.json (2)
7-11: Strengthen validation: enforce non-empty idAdd minLength to ensure id is not an empty string.
"id": { "description": "Shot ID matching the .dat file", - "type": "string" + "type": "string", + "minLength": 1 },
45-48: Strengthen validation: constrain timestamp to non-negative integersHelps catch bad data and aligns with epoch semantics.
"timestamp": { "description": "Timestamp when notes were last updated", - "type": "integer" + "type": "integer", + "minimum": 0 }docs/shot-notes-api.md (2)
11-19: Clarify extended recording triggers and terminology
- Replace “volume” with “weight” where applicable to reflect scale readings.
- Note that brew:prestart is emitted prior to brew:start and intended only for brew mode.
-When brewing with a connected Bluetooth scale (regardless of volumetric or time-based mode), the system now continues recording for a brief period after the shot officially ends to capture the additional liquid that drips from the portafilter. +When brewing with a connected Bluetooth scale (regardless of volumetric or time-based mode), the system now continues recording for a brief period after the shot officially ends to capture additional liquid (post-brew drips) and stabilize the final weight. @@ -**Weight Stabilization**: Stops early if weight doesn't change by more than 0.1g for 1 second +**Weight Stabilization**: Stops early if weight changes by less than 0.1 g over 1 secondAdditionally, consider adding:
- “controller:brew:prestart is emitted only in brew mode prior to controller:brew:start.”
164-166: Terminology nit: “dose out” pre-fill source is weightThe UI pre-fills doseOut from the final weight measurement (not “volume”). Suggest rewording for precision.
-The dose out field is automatically pre-populated with the final volume measurement from the shot data. +The dose out field is automatically pre-populated with the final weight measurement from the shot data.web/src/pages/ShotHistory/HistoryCard.jsx (1)
17-17: useCallback dependency onshotmay cause unnecessary re-createsIf
shotis recreated frequently by the parent, onExport will be re-created as well. If feasible, depend on stable primitives needed by the callback (e.g.,shot.id,shot.timestamp) and access the latestshotat call time, or switch to an inline handler since the function is small.Apply this diff if you want to minimize re-creates while still including the latest shot payload:
- const onExport = useCallback(() => { + const onExport = useCallback(() => { const dataStr = 'data:text/json;charset=utf-8,' + encodeURIComponent(JSON.stringify(shot, undefined, 2)); const downloadAnchorNode = document.createElement('a'); downloadAnchorNode.setAttribute('href', dataStr); downloadAnchorNode.setAttribute('download', shot.id + '-complete.json'); document.body.appendChild(downloadAnchorNode); // required for firefox downloadAnchorNode.click(); downloadAnchorNode.remove(); - }, [shot]); + }, [shot.id, shot.timestamp]);web/src/pages/ShotHistory/ShotNotesCard.jsx (2)
29-59: Effect dependencies look correct; minor UX improvement on pre-fillPre-filling doseOut with shot.volume is great. Consider also pre-filling doseIn if you have a corresponding source (e.g., current dosing target) in shot data in future.
61-78: Persist notes with minimal serialization overhead and surfacing save errorsCurrent flow is fine. Consider surfacing save failures in the UI (toast/snackbar) and disabling Save while there are invalid numeric entries (e.g., negative values).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/shot-notes-api.md(1 hunks)schema/shot_notes.json(1 hunks)src/display/core/Controller.cpp(1 hunks)src/display/plugins/BLEScalePlugin.cpp(3 hunks)src/display/plugins/ShotHistoryPlugin.cpp(6 hunks)src/display/plugins/ShotHistoryPlugin.h(4 hunks)web/src/pages/ShotHistory/HistoryCard.jsx(3 hunks)web/src/pages/ShotHistory/ShotNotesCard.jsx(1 hunks)web/src/pages/ShotHistory/utils.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/display/core/Controller.cpp (1)
src/display/core/Controller.h (1)
pluginManager(127-127)
web/src/pages/ShotHistory/ShotNotesCard.jsx (1)
src/display/plugins/ShotHistoryPlugin.cpp (4)
loadNotes(283-290)loadNotes(283-283)saveNotes(273-281)saveNotes(273-273)
web/src/pages/ShotHistory/HistoryCard.jsx (1)
web/src/pages/ShotHistory/ShotNotesCard.jsx (1)
ShotNotesCard(4-279)
src/display/plugins/ShotHistoryPlugin.cpp (1)
web/src/pages/ShotHistory/ShotNotesCard.jsx (2)
notes(7-16)saveNotes(61-78)
src/display/plugins/ShotHistoryPlugin.h (2)
web/src/pages/ShotHistory/ShotNotesCard.jsx (2)
saveNotes(61-78)notes(7-16)src/display/plugins/ShotHistoryPlugin.cpp (6)
saveNotes(273-281)saveNotes(273-273)loadNotes(283-290)loadNotes(283-283)finalizeRecording(179-188)finalizeRecording(179-179)
🪛 LanguageTool
docs/shot-notes-api.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...ortafilter. Extended Recording Logic: - Trigger: Automatically activates when ...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...if weight doesn't change by more than 0.1g for 1 second - Purpose: Captures mo...
(QB_NEW_EN)
[style] ~27-~27: For conciseness, consider replacing this expression with an adverb.
Context: ... extraction weight, not just the weight at the moment the shot timer stops. ## Enhanced Shot...
(AT_THE_MOMENT)
[grammar] ~33-~33: There might be a mistake here.
Context: ...en available. ### Get Shot History List Request Type: req:history:list **Re...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...} ] } ``` ### Get Single Shot History Request Type: req:history:get **Res...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ... Notes API Endpoints ### Get Shot Notes Request Type: req:history:notes:get ...
(QB_NEW_EN)
[grammar] ~117-~117: There might be a mistake here.
Context: ...692123456 } } ``` ### Save Shot Notes Request Type: req:history:notes:save...
(QB_NEW_EN)
[grammar] ~156-~156: There might be a mistake here.
Context: ... new ShotNotesCard component provides: - Star rating system (1-5 stars) - Dose in...
(QB_NEW_EN)
[grammar] ~157-~157: There might be a mistake here.
Context: ...ovides: - Star rating system (1-5 stars) - Dose in/out fields with automatic ratio ...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ... fields with automatic ratio calculation - Grind setting input - Balance/taste sele...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ... ratio calculation - Grind setting input - Balance/taste selector (bitter, balanced...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: .../taste selector (bitter, balanced, sour) - Free-form notes text area - Edit/save fu...
(QB_NEW_EN)
[grammar] ~161-~161: There might be a mistake here.
Context: ...anced, sour) - Free-form notes text area - Edit/save functionality The dose out fi...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...ced to automatically include notes data: - Export filename: `{shot-id}-complete.jso...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ... include notes data: - Export filename: {shot-id}-complete.json - Contains both shot history data and note...
(QB_NEW_EN)
[grammar] ~201-~201: There might be a mistake here.
Context: ...ma defined in /schema/shot_notes.json: - id: Shot ID (required) - rating: Star ra...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ..._notes.json: - id: Shot ID (required) - rating: Star rating 0-5 - doseIn`: Input dose...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...D (required) - rating: Star rating 0-5 - doseIn: Input dose in grams - doseOut: Outpu...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...ting 0-5 - doseIn: Input dose in grams - doseOut: Output dose in grams - ratio: Calcul...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ... grams - doseOut: Output dose in grams - ratio: Calculated ratio (doseOut/doseIn) - `g...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ...atio: Calculated ratio (doseOut/doseIn) - grindSetting`: String description of grind setting - ...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ...ng: String description of grind setting - balanceTaste`: One of "bitter", "balanced", "sour" - ...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...te: One of "bitter", "balanced", "sour" - notes: Free-form text notes - timestamp`: Wh...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ..., "sour" - notes: Free-form text notes - timestamp: When notes were last updated
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ...timestamp`: When notes were last updated
(QB_NEW_EN)
🔇 Additional comments (6)
web/src/pages/ShotHistory/HistoryCard.jsx (3)
4-4: Notes UI integration looks goodImporting and integrating ShotNotesCard here is cohesive with the new notes workflow. No concerns.
13-13: Export filename change aligns with “complete” payloadRenaming to -complete.json is appropriate given the export now includes notes. No issues.
61-61: LGTM: Rendering ShotNotesCard below the chartPlacement and prop passing are clear and consistent.
src/display/plugins/ShotHistoryPlugin.h (1)
11-13: Constants align with PR objectiveEXTENDED_RECORDING_DURATION and stabilization thresholds match the stated behavior (3s or stabilize <0.1g in 1s).
web/src/pages/ShotHistory/ShotNotesCard.jsx (1)
151-256: Good separation of edit vs view modes and accessible controlsThe edit controls, star rating, and read-only placeholders are well-implemented. No issues.
src/display/plugins/ShotHistoryPlugin.cpp (1)
183-183: Nice touch: cleanup notes on short/invalid shotsDeleting the notes alongside .dat for very short shots is consistent and prevents stale notes.
src/display/core/Controller.cpp
Outdated
| if (isVolumetricAvailable()) | ||
| pluginManager->trigger("controller:brew:prestart"); | ||
| delay(100); |
There was a problem hiding this comment.
Restrict prestart emission to brew mode to avoid unintended taring in other modes
Currently, prestart fires for any activation when volumetric is available (including steam/water start), which can unnecessarily tare scales. Emit prestart only when starting brew. Combined with BLE plugin’s brew:start fallback (recommended in BLEScalePlugin), this prevents regressions.
- if (isVolumetricAvailable())
- pluginManager->trigger("controller:brew:prestart");
+ if (mode == MODE_BREW && isVolumetricAvailable())
+ pluginManager->trigger("controller:brew:prestart");📝 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.
| if (isVolumetricAvailable()) | |
| pluginManager->trigger("controller:brew:prestart"); | |
| delay(100); | |
| if (mode == MODE_BREW && isVolumetricAvailable()) | |
| pluginManager->trigger("controller:brew:prestart"); | |
| delay(100); |
🤖 Prompt for AI Agents
In src/display/core/Controller.cpp around lines 503 to 505, the prestart event
is emitted whenever volumetric is available which causes unwanted taring in
non-brew activations; change the condition so
pluginManager->trigger("controller:brew:prestart") is only called for actual
brew starts (e.g. add an explicit check for the controller's current mode/state
being BREW or for the activation type indicating a brew start) rather than just
isVolumetricAvailable(), ensuring other activation paths (steam/water) do not
fire prestart; keep the existing volumetric availability check as part of the
combined condition and retain the BLE plugin’s recommended brew:start fallback
behavior.
| @@ -32,6 +33,7 @@ void BLEScalePlugin::setup(Controller *controller, PluginManager *manager) { | |||
| TimemoreScalesPlugin::apply(); | |||
| VariaScalesPlugin::apply(); | |||
| WeighMyBrewScalePlugin::apply(); | |||
| myscalePlugin::apply(); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify plugin symbol casing and naming consistency for myscale
Other scale plugins use PascalCase type names (e.g., AcaiaScalesPlugin). Please confirm the correct symbol is myscalePlugin::apply() (lowercase) and not MyScalePlugin::apply() (or similar). If mismatched, it will fail to link.
Run the following to verify headers and apply symbol exist:
🏁 Script executed:
#!/bin/bash
# Check header presence
fd -a 'myscale.h'
# Search for apply() symbol variants
rg -n --no-heading -A2 -B2 $'myscalePlugin::apply|MyScalePlugin::apply|MyScale::apply|myscale::apply|MyscalePlugin::apply'Length of output: 559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for any 'myscale' (case-insensitive)..."
rg -n -S -i 'myscale' || true
echo
echo "Searching common casing variants (MyScale, Myscale, myScale)..."
rg -n --no-heading -S 'MyScale|Myscale|myScale' || true
echo
echo "Searching for any '::apply(' usages to see plugin apply patterns..."
rg -n --no-heading '::apply\(' || true
echo
echo "Searching for ScalePlugin class declarations..."
rg -n --no-heading -S 'class .*ScalePlugin' || true
echo
echo "Searching for likely header names for myscale plugin..."
rg -n --no-heading -S 'myscale.h|myscale.hpp|myscale_plugin.h|MyScalePlugin' || trueLength of output: 2040
Action required: fix plugin symbol casing for myscale (will fail to link)
Short: A repo-wide search found only the include (<scales/myscale.h>) and the call myscalePlugin::apply() — no definition for a symbol named "myscalePlugin" was found. Other plugins use PascalCase (e.g., AcaiaScalesPlugin), so this looks like a naming/casing mismatch that will break linking.
Locations:
- src/display/plugins/BLEScalePlugin.cpp — line ~15: #include <scales/myscale.h>
- src/display/plugins/BLEScalePlugin.cpp — line 36: myscalePlugin::apply();
Current snippet:
myscalePlugin::apply();
Suggested fix (use the exact class name exported by scales/myscale.h or define the missing symbol):
<CorrectPluginClassName>::apply(); // e.g. MyScalePlugin::apply() or MyScaleScalesPlugin::apply()
Action items:
- Open scales/myscale.h and use the exact exported class/type name in the call, or
- Define the missing myscalePlugin symbol (prefer consistent PascalCase naming to match other plugins).
🤖 Prompt for AI Agents
In src/display/plugins/BLEScalePlugin.cpp around line 36, the call uses a
lowercase symbol "myscalePlugin::apply()" which doesn't match any exported
symbol from scales/myscale.h and will break linking; open scales/myscale.h, note
the exact exported class/type name (e.g., MyScalePlugin or MyScaleScalesPlugin)
and replace the call with that exact PascalCase name followed by ::apply(), or
if the header truly lacks the symbol, add/define the correctly named plugin
class in the scales/myscale.h implementation to match the project’s PascalCase
convention.
| @@ -40,7 +42,7 @@ void BLEScalePlugin::setup(Controller *controller, PluginManager *manager) { | |||
| active = true; | |||
| } | |||
| }); | |||
| manager->on("controller:brew:start", [this](Event const &) { onProcessStart(); }); | |||
| manager->on("controller:brew:prestart", [this](Event const &) { onProcessStart(); }); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also listen to brew:start to avoid regressions when prestart isn’t emitted
Prestart is only triggered when volumetric is available (per Controller.cpp). If volumetric isn’t available, your taring won’t run anymore because you removed the brew:start listener. Safer approach: listen to both prestart and brew:start.
Apply this diff to restore brew:start handling:
- manager->on("controller:brew:prestart", [this](Event const &) { onProcessStart(); });
+ manager->on("controller:brew:prestart", [this](Event const &) { onProcessStart(); });
+ manager->on("controller:brew:start", [this](Event const &) { onProcessStart(); });📝 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.
| manager->on("controller:brew:prestart", [this](Event const &) { onProcessStart(); }); | |
| manager->on("controller:brew:prestart", [this](Event const &) { onProcessStart(); }); | |
| manager->on("controller:brew:start", [this](Event const &) { onProcessStart(); }); |
🤖 Prompt for AI Agents
In src/display/plugins/BLEScalePlugin.cpp around line 45, you removed the
brew:start listener and now only listen to controller:brew:prestart which only
fires when volumetric is available; restore handling by also registering a
listener for "controller:brew:start" that calls onProcessStart() (i.e., add a
second manager->on(...) for "controller:brew:start" mirroring the existing
prestart handler) so taring still runs when prestart isn’t emitted.
| String id = request["id"].as<String>(); | ||
| SPIFFS.remove("/h/" + id + ".dat"); | ||
| SPIFFS.remove("/h/" + id + ".json"); // Also remove notes file if it exists | ||
| response["msg"] = "Ok"; | ||
| } else if (type == "req:history:notes:get") { | ||
| String id = request["id"].as<String>(); | ||
| JsonDocument notes; | ||
| loadNotes(id, notes); | ||
| response["notes"] = notes; | ||
| } else if (type == "req:history:notes:save") { | ||
| String id = request["id"].as<String>(); | ||
| JsonDocument notes = request["notes"]; | ||
| notes["timestamp"] = getTime(); | ||
| saveNotes(id, notes); | ||
| response["msg"] = "Ok"; |
There was a problem hiding this comment.
Notes save/get handlers: avoid JsonDocument construction and set timestamp safely
Use JsonVariant/JsonObject to avoid dynamic allocations you don’t control.
Apply this diff:
- } else if (type == "req:history:notes:get") {
- String id = request["id"].as<String>();
- JsonDocument notes;
- loadNotes(id, notes);
- response["notes"] = notes;
- } else if (type == "req:history:notes:save") {
- String id = request["id"].as<String>();
- JsonDocument notes = request["notes"];
- notes["timestamp"] = getTime();
- saveNotes(id, notes);
+ } else if (type == "req:history:notes:get") {
+ String id = request["id"].as<String>();
+ if (!loadNotes(id, response["notes"])) {
+ response["notes"] = JsonObject(); // empty object if not found
+ }
+ } else if (type == "req:history:notes:save") {
+ String id = request["id"].as<String>();
+ JsonObject notes = request["notes"].as<JsonObject>();
+ if (notes.isNull()) {
+ response["error"] = "invalid notes";
+ return;
+ }
+ notes["timestamp"] = getTime();
+ saveNotes(id, notes);
response["msg"] = "Ok";📝 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.
| String id = request["id"].as<String>(); | |
| SPIFFS.remove("/h/" + id + ".dat"); | |
| SPIFFS.remove("/h/" + id + ".json"); // Also remove notes file if it exists | |
| response["msg"] = "Ok"; | |
| } else if (type == "req:history:notes:get") { | |
| String id = request["id"].as<String>(); | |
| JsonDocument notes; | |
| loadNotes(id, notes); | |
| response["notes"] = notes; | |
| } else if (type == "req:history:notes:save") { | |
| String id = request["id"].as<String>(); | |
| JsonDocument notes = request["notes"]; | |
| notes["timestamp"] = getTime(); | |
| saveNotes(id, notes); | |
| response["msg"] = "Ok"; | |
| String id = request["id"].as<String>(); | |
| SPIFFS.remove("/h/" + id + ".dat"); | |
| SPIFFS.remove("/h/" + id + ".json"); // Also remove notes file if it exists | |
| response["msg"] = "Ok"; | |
| } else if (type == "req:history:notes:get") { | |
| String id = request["id"].as<String>(); | |
| if (!loadNotes(id, response["notes"])) { | |
| response["notes"] = JsonObject(); // empty object if not found | |
| } | |
| } else if (type == "req:history:notes:save") { | |
| String id = request["id"].as<String>(); | |
| JsonObject notes = request["notes"].as<JsonObject>(); | |
| if (notes.isNull()) { | |
| response["error"] = "invalid notes"; | |
| return; | |
| } | |
| notes["timestamp"] = getTime(); | |
| saveNotes(id, notes); | |
| response["msg"] = "Ok"; |
🤖 Prompt for AI Agents
In src/display/plugins/ShotHistoryPlugin.cpp around lines 255 to 269, the
handlers currently construct JsonDocument objects directly; change them to use
JsonVariant/JsonObject so we don't perform uncontrolled dynamic allocations and
to safely set the timestamp: in the "req:history:notes:get" handler accept/load
into a JsonVariant/JsonObject (e.g. JsonVariant notes or JsonObject notes =
request["notes"].as<JsonObject>() or adjust loadNotes signature to take a
JsonVariant/JsonObject) and assign response["notes"] = notes; in the
"req:history:notes:save" handler read the incoming notes as a JsonObject
(JsonObject notes = request["notes"].as<JsonObject>()), set notes["timestamp"] =
getTime() on that object, and call saveNotes(id, notes) (update saveNotes
signature to accept a JsonVariant/JsonObject if needed); also avoid declaring
JsonDocument variables in these handlers and update loadNotes/saveNotes
signatures accordingly.
| void saveNotes(const String &id, const JsonDocument ¬es); | ||
| void loadNotes(const String &id, JsonDocument ¬es); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ArduinoJson API misuse: JsonDocument in interfaces will cause compilation/runtime issues
ArduinoJson v6 doesn’t allow constructing a plain JsonDocument; using it in interfaces forces callers to allocate a document and pass it, which your .cpp currently does incorrectly. Prefer JsonVariant* based APIs to avoid separate allocations and to deserialize directly into the response document’s memory pool.
Apply this diff to the declarations:
- void saveNotes(const String &id, const JsonDocument ¬es);
- void loadNotes(const String &id, JsonDocument ¬es);
+ void saveNotes(const String &id, JsonVariantConst notes);
+ bool loadNotes(const String &id, JsonVariant dst);📝 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.
| void saveNotes(const String &id, const JsonDocument ¬es); | |
| void loadNotes(const String &id, JsonDocument ¬es); | |
| void saveNotes(const String &id, JsonVariantConst notes); | |
| bool loadNotes(const String &id, JsonVariant dst); |
🤖 Prompt for AI Agents
In src/display/plugins/ShotHistoryPlugin.h around lines 27-28, the signatures
using JsonDocument are incorrect for ArduinoJson v6; change the API to accept a
read-only JsonVariantConst (or JsonVariantConst&) for input (saveNotes) and a
writable JsonVariant (or JsonVariant&) for output (loadNotes) so callers can
pass variants that point into their JsonDocument memory pool; update both
declaration lines accordingly and adjust the .cpp to deserialize/serialize
directly into the provided JsonVariant (or variant reference) rather than
attempting to construct a JsonDocument in the interface.
| bool extendedRecording = false; | ||
| unsigned long shotStart = 0; | ||
| unsigned long extendedRecordingStart = 0; | ||
| unsigned long lastVolumeSample = 0; | ||
| unsigned long lastWeightChangeTime = 0; | ||
| float currentTemperature = 0.0f; | ||
| float currentBluetoothWeight = 0.0f; | ||
| float lastStableWeight = 0.0f; | ||
| float currentBluetoothFlow = 0.0f; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Shared state accessed across ISR/event and FreeRTOS task without synchronization
Members like currentBluetoothWeight, currentBluetoothFlow, lastVolumeSample, recording, extendedRecording, lastStableWeight, lastWeightChangeTime are written in event callbacks and read in loopTask/record(). On ESP32 (dual core), this is a data race risk.
At minimum mark shared scalars volatile; ideally protect updates/reads with a critical section or a mutex if contention becomes observable.
Do you want me to propose a minimal critical-section wrapper for the weight fields?
🤖 Prompt for AI Agents
In src/display/plugins/ShotHistoryPlugin.h around lines 63 to 71, several scalar
members (currentBluetoothWeight, currentBluetoothFlow, lastVolumeSample,
recording/extendedRecording flags, lastStableWeight, lastWeightChangeTime) are
written from ISRs/event callbacks and read from loopTask/record() causing data
races on dual-core ESP32; mark these shared scalar fields volatile and wrap all
multi-field reads/writes (and any non-atomic updates) in a critical section or
mutex to prevent tear/race — for minimal change declare the listed fields
volatile, protect event-callback updates and the loopTask/record() reads with a
short critical section (or a FreeRTOS mutex if blocking is acceptable), and keep
critical sections as small as possible around the exact assignments/reads to
avoid scheduling delays.
| // Calculate ratio when doseIn or doseOut changes | ||
| useEffect(() => { | ||
| if (notes.doseIn && notes.doseOut) { | ||
| const ratio = (parseFloat(notes.doseOut) / parseFloat(notes.doseIn)).toFixed(2); | ||
| setNotes(prev => ({ ...prev, ratio })); | ||
| } | ||
| }, [notes.doseIn, notes.doseOut]); |
There was a problem hiding this comment.
Guard against divide-by-zero and NaN when computing ratio
If doseIn is "0" or non-numeric, ratio becomes Infinity/NaN. Add validation before computing.
Apply this diff:
- useEffect(() => {
- if (notes.doseIn && notes.doseOut) {
- const ratio = (parseFloat(notes.doseOut) / parseFloat(notes.doseIn)).toFixed(2);
- setNotes(prev => ({ ...prev, ratio }));
- }
- }, [notes.doseIn, notes.doseOut]);
+ useEffect(() => {
+ const inVal = parseFloat(notes.doseIn);
+ const outVal = parseFloat(notes.doseOut);
+ if (!Number.isNaN(inVal) && !Number.isNaN(outVal) && inVal > 0) {
+ const ratio = (outVal / inVal).toFixed(2);
+ setNotes(prev => ({ ...prev, ratio }));
+ } else {
+ setNotes(prev => ({ ...prev, ratio: '' }));
+ }
+ }, [notes.doseIn, notes.doseOut]);📝 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.
| // Calculate ratio when doseIn or doseOut changes | |
| useEffect(() => { | |
| if (notes.doseIn && notes.doseOut) { | |
| const ratio = (parseFloat(notes.doseOut) / parseFloat(notes.doseIn)).toFixed(2); | |
| setNotes(prev => ({ ...prev, ratio })); | |
| } | |
| }, [notes.doseIn, notes.doseOut]); | |
| // Calculate ratio when doseIn or doseOut changes | |
| useEffect(() => { | |
| const inVal = parseFloat(notes.doseIn); | |
| const outVal = parseFloat(notes.doseOut); | |
| if (!Number.isNaN(inVal) && !Number.isNaN(outVal) && inVal > 0) { | |
| const ratio = (outVal / inVal).toFixed(2); | |
| setNotes(prev => ({ ...prev, ratio })); | |
| } else { | |
| setNotes(prev => ({ ...prev, ratio: '' })); | |
| } | |
| }, [notes.doseIn, notes.doseOut]); |
🤖 Prompt for AI Agents
In web/src/pages/ShotHistory/ShotNotesCard.jsx around lines 21 to 27, the ratio
calculation does not guard against doseIn being "0" or non-numeric which can
produce Infinity or NaN; update the effect to parse doseIn and doseOut into
numbers, verify both are finite and doseIn !== 0 before computing
parseFloat(doseOut)/parseFloat(doseIn) and toFixed(2), and otherwise set ratio
to a safe default (e.g., empty string or null) via setNotes so you never store
Infinity/NaN.
|
Please replace all the try...catch with explicit error handling. In embedded builds, the exception mechanism is implementation-dependent and may not work as expected because there is nothing to throw an exception. |
dragm83
force-pushed
the
shot_record_fix
branch
from
8547d18 to
0583ed9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/display/plugins/ShotHistoryPlugin.cpp (1)
18-34: Good start on removing try-catch, but weight validation needs improvement.The removal of try-catch blocks aligns with the PR feedback for embedded systems. However, the current validation has a few issues:
- Use
std::isnanorisnanffrom<cmath>instead ofisnanfor portability- Add protection against division by zero when calculating flow
Apply this diff to improve the validation:
+#include <cmath> // ... other includes ... pm->on("controller:volumetric-measurement:bluetooth:change", [this](Event const &event) { // Explicit checks instead of try/catch for embedded systems const float weight = event.getFloat("value"); // Validate weight data before processing - if (isnan(weight) || weight < 0) { + if (std::isnan(weight) || weight < 0) { return; // Skip invalid weight data } const unsigned long now = millis(); if (lastVolumeSample != 0) { const unsigned long timeDiff = now - lastVolumeSample; + if (timeDiff == 0) { + return; // Avoid division by zero + } const float volumeDiff = weight - currentBluetoothWeight; currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; } lastVolumeSample = now; currentBluetoothWeight = weight; });
♻️ Duplicate comments (6)
src/display/plugins/ShotHistoryPlugin.h (2)
27-28: ArduinoJson API issue persists - use JsonVariant for proper memory management.The
JsonDocumenttype shouldn't be used in function signatures with ArduinoJson v6. This will cause compilation issues or runtime failures.Apply this diff to fix the signatures:
- void saveNotes(const String &id, const JsonDocument ¬es); - void loadNotes(const String &id, JsonDocument ¬es); + void saveNotes(const String &id, JsonVariantConst notes); + bool loadNotes(const String &id, JsonVariant dst);
63-71: Shared state needs synchronization for thread safety on ESP32.The members
currentBluetoothWeight,currentBluetoothFlow,lastVolumeSample,recording,extendedRecording,lastStableWeight, andlastWeightChangeTimeare accessed from both event callbacks and the FreeRTOS task without synchronization, creating data race risks on the dual-core ESP32.At minimum, mark these shared fields as
volatile. For better thread safety, consider using a FreeRTOS mutex or critical sections to protect multi-field updates:- bool recording = false; - bool headerWritten = false; - bool extendedRecording = false; - unsigned long shotStart = 0; - unsigned long extendedRecordingStart = 0; - unsigned long lastVolumeSample = 0; - unsigned long lastWeightChangeTime = 0; - float currentTemperature = 0.0f; - float currentBluetoothWeight = 0.0f; - float lastStableWeight = 0.0f; - float currentBluetoothFlow = 0.0f; + volatile bool recording = false; + volatile bool headerWritten = false; + volatile bool extendedRecording = false; + volatile unsigned long shotStart = 0; + volatile unsigned long extendedRecordingStart = 0; + volatile unsigned long lastVolumeSample = 0; + volatile unsigned long lastWeightChangeTime = 0; + volatile float currentTemperature = 0.0f; + volatile float currentBluetoothWeight = 0.0f; + volatile float lastStableWeight = 0.0f; + volatile float currentBluetoothFlow = 0.0f;src/display/plugins/ShotHistoryPlugin.cpp (4)
89-89: Use the correct floating-point absolute value function.
absmay resolve to the integer overload. Usestd::fabsorfabsffor floating-point values.Apply this diff:
- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = std::fabs(currentBluetoothWeight - lastStableWeight);Also ensure
#include <cmath>is present at the top of the file.
153-159: Extended recording condition may be too restrictive.The current check requires
controller->isVolumetricAvailable(), which might prevent extended recording even when BLE weight data is present. According to the PR objectives, the goal is to capture post-brew drip whenever weight is available, not only when volumetric mode is active.Consider checking for recent weight updates instead:
- // Check if we have active bluetooth weight data (regardless of volumetric mode) - bool hasActiveWeightData = false; - - if (controller != nullptr) { - if (controller->isVolumetricAvailable() && currentBluetoothWeight > 0) { - hasActiveWeightData = true; - } - } + // Check if we have active bluetooth weight data (regardless of volumetric mode) + const unsigned long now = millis(); + const bool hasRecentSample = lastVolumeSample != 0 && (now - lastVolumeSample) <= 1000; + const bool hasActiveWeightData = controller != nullptr && hasRecentSample && currentBluetoothWeight > 0.0f;This aligns better with the PR's goal of "continue recording for 3s or until weight stabilizes" whenever weight data is available.
268-285: Refactor saveNotes/loadNotes to use JsonVariant for proper ArduinoJson v6 compatibility.The current implementation uses
JsonDocumentin signatures and creates intermediate strings unnecessarily.Apply this refactor to align with ArduinoJson v6 best practices:
-void ShotHistoryPlugin::saveNotes(const String &id, const JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); - if (file) { - String notesStr; - serializeJson(notes, notesStr); - file.print(notesStr); - file.close(); - } -} +void ShotHistoryPlugin::saveNotes(const String &id, JsonVariantConst notes) { + File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); + if (!file) return; + serializeJson(notes, file); + file.close(); +} -void ShotHistoryPlugin::loadNotes(const String &id, JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", "r"); - if (file) { - String notesStr = file.readString(); - file.close(); - deserializeJson(notes, notesStr); - } -} +bool ShotHistoryPlugin::loadNotes(const String &id, JsonVariant dst) { + File file = SPIFFS.open("/h/" + id + ".json", "r"); + if (!file) return false; + DeserializationError err = deserializeJson(dst, file); + file.close(); + return !err; +}This eliminates intermediate string allocations and properly returns success/failure status.
254-264: Fix JsonDocument usage in notes endpoints.The notes get/save handlers create unnecessary
JsonDocumentinstances. UseJsonVariantfor better memory management.Apply this diff:
} else if (type == "req:history:notes:get") { String id = request["id"].as<String>(); - JsonDocument notes; - loadNotes(id, notes); - response["notes"] = notes; + if (!loadNotes(id, response["notes"])) { + response["notes"] = JsonObject(); // empty object if not found + } } else if (type == "req:history:notes:save") { String id = request["id"].as<String>(); - JsonDocument notes = request["notes"]; + JsonObject notes = request["notes"].as<JsonObject>(); + if (notes.isNull()) { + response["error"] = "invalid notes"; + return; + } notes["timestamp"] = getTime(); saveNotes(id, notes); response["msg"] = "Ok";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
docs/shot-notes-api.md(1 hunks)schema/shot_notes.json(1 hunks)src/display/plugins/ShotHistoryPlugin.cpp(7 hunks)src/display/plugins/ShotHistoryPlugin.h(4 hunks)web/src/pages/ShotHistory/HistoryCard.jsx(3 hunks)web/src/pages/ShotHistory/ShotNotesCard.jsx(1 hunks)web/src/pages/ShotHistory/utils.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/pages/ShotHistory/HistoryCard.jsx
- web/src/pages/ShotHistory/ShotNotesCard.jsx
- schema/shot_notes.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/display/plugins/ShotHistoryPlugin.h (2)
src/display/plugins/ShotHistoryPlugin.cpp (6)
saveNotes(268-276)saveNotes(268-268)loadNotes(278-285)loadNotes(278-278)finalizeRecording(174-183)finalizeRecording(174-174)web/src/pages/ShotHistory/ShotNotesCard.jsx (2)
saveNotes(61-78)notes(7-16)
🪛 LanguageTool
docs/shot-notes-api.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...ortafilter. Extended Recording Logic: - Trigger: Automatically activates when ...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...if weight doesn't change by more than 0.1g for 1 second - Purpose: Captures mo...
(QB_NEW_EN)
[style] ~27-~27: For conciseness, consider replacing this expression with an adverb.
Context: ... extraction weight, not just the weight at the moment the shot timer stops. ## Enhanced Shot...
(AT_THE_MOMENT)
[grammar] ~33-~33: There might be a mistake here.
Context: ...en available. ### Get Shot History List Request Type: req:history:list **Re...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...} ] } ``` ### Get Single Shot History Request Type: req:history:get **Res...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ... Notes API Endpoints ### Get Shot Notes Request Type: req:history:notes:get ...
(QB_NEW_EN)
[grammar] ~117-~117: There might be a mistake here.
Context: ...692123456 } } ``` ### Save Shot Notes Request Type: req:history:notes:save...
(QB_NEW_EN)
[grammar] ~156-~156: There might be a mistake here.
Context: ... new ShotNotesCard component provides: - Star rating system (1-5 stars) - Dose in...
(QB_NEW_EN)
[grammar] ~157-~157: There might be a mistake here.
Context: ...ovides: - Star rating system (1-5 stars) - Dose in/out fields with automatic ratio ...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ... fields with automatic ratio calculation - Grind setting input - Balance/taste sele...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ... ratio calculation - Grind setting input - Balance/taste selector (bitter, balanced...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: .../taste selector (bitter, balanced, sour) - Free-form notes text area - Edit/save fu...
(QB_NEW_EN)
[grammar] ~161-~161: There might be a mistake here.
Context: ...anced, sour) - Free-form notes text area - Edit/save functionality The dose out fi...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...ced to automatically include notes data: - Export filename: `{shot-id}-complete.jso...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ... include notes data: - Export filename: {shot-id}-complete.json - Contains both shot history data and note...
(QB_NEW_EN)
[grammar] ~201-~201: There might be a mistake here.
Context: ...ma defined in /schema/shot_notes.json: - id: Shot ID (required) - rating: Star ra...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ..._notes.json: - id: Shot ID (required) - rating: Star rating 0-5 - doseIn`: Input dose...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...D (required) - rating: Star rating 0-5 - doseIn: Input dose in grams - doseOut: Outpu...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...ting 0-5 - doseIn: Input dose in grams - doseOut: Output dose in grams - ratio: Calcul...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ... grams - doseOut: Output dose in grams - ratio: Calculated ratio (doseOut/doseIn) - `g...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ...atio: Calculated ratio (doseOut/doseIn) - grindSetting`: String description of grind setting - ...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ...ng: String description of grind setting - balanceTaste`: One of "bitter", "balanced", "sour" - ...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...te: One of "bitter", "balanced", "sour" - notes: Free-form text notes - timestamp`: Wh...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ..., "sour" - notes: Free-form text notes - timestamp: When notes were last updated
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ...timestamp`: When notes were last updated
(QB_NEW_EN)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (2)
web/src/pages/ShotHistory/utils.js (1)
36-40: LGTM! Clean implementation of notes integration.The conditional addition of notes to the parsed history data is well-implemented and maintains backward compatibility for shots without notes.
docs/shot-notes-api.md (1)
1-211: Excellent documentation! Comprehensive and well-structured.The documentation clearly explains the extended recording feature, API endpoints, data structures, and frontend implementation. The examples are helpful and the schema reference is properly included.
dragm83
force-pushed
the
shot_record_fix
branch
from
0583ed9 to
107dbfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
web/src/pages/ShotHistory/utils.js (1)
31-35: Guard against empty samples to avoid runtime errordata['samples'] is always an array; when it's empty, lastSample becomes undefined and accessing lastSample.t will throw. Check length > 0.
- if (data['samples']) { - const lastSample = data['samples'][data['samples'].length - 1]; - data.duration = lastSample.t; - data.volume = lastSample.v; - } + if (Array.isArray(data['samples']) && data['samples'].length > 0) { + const lastSample = data['samples'][data['samples'].length - 1]; + data.duration = lastSample.t; + data.volume = lastSample.v; + }src/display/plugins/ShotHistoryPlugin.cpp (1)
27-31: Prevent divide-by-zero when two BLE updates occur within the same millisecondtimeDiff can be 0 when events share the same millis() tick; this will divide by zero.
if (lastVolumeSample != 0) { const unsigned long timeDiff = now - lastVolumeSample; + if (timeDiff == 0) { + // Skip flow calculation on identical timestamps + lastVolumeSample = now; + currentBluetoothWeight = weight; + return; + } const float volumeDiff = weight - currentBluetoothWeight; currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; }
♻️ Duplicate comments (7)
src/display/plugins/ShotHistoryPlugin.cpp (7)
76-87: Don't gate extended recording on controller->isVolumetricAvailable(); rely on recent weight samplesRequiring volumetric availability can skip valid post-brew drip when BLE scale updates are present. Use recency of lastVolumeSample instead and end early only when no recent sample exists.
- // Explicit safety checks instead of try/catch for embedded systems - bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } - - if (!canProcessWeight) { - // If BLE connection is unstable, end extended recording early - extendedRecording = false; - finalizeRecording(); - return; - } + // Consider weight "active" if we've seen a recent BLE sample + const bool hasRecentSample = lastVolumeSample != 0 && (now - lastVolumeSample) <= 1000; + if (!hasRecentSample) { + // If scale updates stop, end extended recording early + extendedRecording = false; + finalizeRecording(); + return; + }
152-168: Trigger extended recording based on recent BLE sample, not volumetric availabilityAt shot end, extended recording should start whenever the scale has been updating recently and weight > 0, regardless of volumetric mode.
- // Check if we have active bluetooth weight data (regardless of volumetric mode) - bool hasActiveWeightData = false; - - if (controller != nullptr) { - if (controller->isVolumetricAvailable() && currentBluetoothWeight > 0) { - hasActiveWeightData = true; - } - } + // Start extended recording if BLE weight was updated recently and is > 0 + const unsigned long now = millis(); + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + const bool hasActiveWeightData = hasRecentSample && currentBluetoothWeight > 0.0f;
268-285: Refactor saveNotes/loadNotes to use JsonVariant and stream directly to SPIFFSThis avoids intermediate String/JsonDocument allocations and reduces fragmentation.
-void ShotHistoryPlugin::saveNotes(const String &id, const JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); - if (file) { - String notesStr; - serializeJson(notes, notesStr); - file.print(notesStr); - file.close(); - } -} +void ShotHistoryPlugin::saveNotes(const String &id, JsonVariantConst notes) { + File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); + if (!file) return; + serializeJson(notes, file); + file.close(); +} -void ShotHistoryPlugin::loadNotes(const String &id, JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", "r"); - if (file) { - String notesStr = file.readString(); - file.close(); - deserializeJson(notes, notesStr); - } -} +bool ShotHistoryPlugin::loadNotes(const String &id, JsonVariant dst) { + File file = SPIFFS.open("/h/" + id + ".json", "r"); + if (!file) return false; + DeserializationError err = deserializeJson(dst, file); + file.close(); + return !err; +}Remember to update the declarations in ShotHistoryPlugin.h accordingly.
89-89: Use fabsf/std::fabs for floating-point absolute valueabs on floats can pick the integer overload. Use fabsf or std::fabs.
- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);Also ensure is included (see earlier comment).
223-227: ArduinoJson: avoid intermediate JsonDocument; deserialize notes directly into responseConstructing a separate JsonDocument and assigning it can cause allocation issues on embedded targets. Load directly into o["notes"].
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - o["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, o["notes"])) { + o.remove("notes"); + }Note: Requires changing loadNotes signature as suggested below.
240-245: Same ArduinoJson issue in get endpoint: deserialize into response["notes"]Load directly into response["notes"] to keep ownership in the same document.
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - response["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, response["notes"])) { + response.remove("notes"); + }
254-265: Notes endpoints: avoid JsonDocument construction; validate input and set timestamp safelyUse JsonObject/JsonVariant to avoid extra allocations and allow in-place timestamping.
- } else if (type == "req:history:notes:get") { - String id = request["id"].as<String>(); - JsonDocument notes; - loadNotes(id, notes); - response["notes"] = notes; - } else if (type == "req:history:notes:save") { - String id = request["id"].as<String>(); - JsonDocument notes = request["notes"]; - notes["timestamp"] = getTime(); - saveNotes(id, notes); + } else if (type == "req:history:notes:get") { + String id = request["id"].as<String>(); + if (!loadNotes(id, response["notes"])) { + response["notes"] = JsonObject(); // empty if not found + } + } else if (type == "req:history:notes:save") { + String id = request["id"].as<String>(); + JsonObject notes = request["notes"].as<JsonObject>(); + if (notes.isNull()) { + response["error"] = "invalid notes"; + return; + } + notes["timestamp"] = getTime(); + saveNotes(id, notes); response["msg"] = "Ok";
🧹 Nitpick comments (4)
src/display/plugins/ShotHistoryPlugin.cpp (1)
21-24: Use finite check and includeisnan() misses infinities; use std::isfinite and add . Also compare against 0.0f.
+#include <cmath> @@ - // Validate weight data before processing - if (isnan(weight) || weight < 0) { + // Validate weight data before processing + if (!std::isfinite(weight) || weight < 0.0f) { return; // Skip invalid weight data }docs/shot-notes-api.md (3)
11-18: Clarify extended recording trigger and stability criteriaThe current text is good, but two precision tweaks improve accuracy and alignment with implementation:
- Explicitly say “weight is finite and > 0”
- Use “weight” consistently (instead of “volume” where grams are implied)
-When brewing with a connected Bluetooth scale (regardless of volumetric or time-based mode), the system now continues recording for a brief period after the shot officially ends to capture the additional liquid that drips from the portafilter. +When brewing with a connected Bluetooth scale (regardless of volumetric or time-based mode), the system continues recording briefly after the shot ends to capture additional liquid that drips from the portafilter. @@ -**Trigger**: Automatically activates when Bluetooth scale data is available and weight > 0 +**Trigger**: Automatically activates when Bluetooth scale data is available and weight is finite and > 0 @@ -**Weight Stabilization**: Stops early if weight doesn't change by more than 0.1g for 1 second +**Weight Stabilization**: Stops early if weight changes by less than 0.1 g over 1 second
164-165: “Dose out” prefill wording: use weight instead of volumeDoseOut is in grams; “final weight measurement” is clearer than “final volume.”
-The dose out field is automatically pre-populated with the final volume measurement from the shot data. +The dose out field is automatically pre-populated with the final weight measurement from the shot data.
20-25: Optional: document constants as parameters, not hard-coded valuesIf these constants are configurable or may evolve, prefer describing them without literal values here and link to the header for the authoritative source.
-constexpr unsigned long EXTENDED_RECORDING_DURATION = 3000; // 3 seconds max -constexpr unsigned long WEIGHT_STABILIZATION_TIME = 1000; // 1 second stability -constexpr float WEIGHT_STABILIZATION_THRESHOLD = 0.1f; // 0.1g threshold +// See src/display/plugins/ShotHistoryPlugin.h for authoritative values: +// EXTENDED_RECORDING_DURATION (max post-shot recording window) +// WEIGHT_STABILIZATION_TIME (stability window) +// WEIGHT_STABILIZATION_THRESHOLD (stability threshold)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
docs/shot-notes-api.md(1 hunks)schema/shot_notes.json(1 hunks)src/display/plugins/ShotHistoryPlugin.cpp(7 hunks)src/display/plugins/ShotHistoryPlugin.h(4 hunks)web/src/pages/ShotHistory/HistoryCard.jsx(3 hunks)web/src/pages/ShotHistory/ShotNotesCard.jsx(1 hunks)web/src/pages/ShotHistory/utils.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/pages/ShotHistory/ShotNotesCard.jsx
- web/src/pages/ShotHistory/HistoryCard.jsx
- src/display/plugins/ShotHistoryPlugin.h
- schema/shot_notes.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/display/plugins/ShotHistoryPlugin.cpp (1)
web/src/pages/ShotHistory/ShotNotesCard.jsx (2)
notes(7-16)saveNotes(61-78)
🪛 LanguageTool
docs/shot-notes-api.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...ortafilter. Extended Recording Logic: - Trigger: Automatically activates when ...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...if weight doesn't change by more than 0.1g for 1 second - Purpose: Captures mo...
(QB_NEW_EN)
[style] ~27-~27: For conciseness, consider replacing this expression with an adverb.
Context: ... extraction weight, not just the weight at the moment the shot timer stops. ## Enhanced Shot...
(AT_THE_MOMENT)
[grammar] ~33-~33: There might be a mistake here.
Context: ...en available. ### Get Shot History List Request Type: req:history:list **Re...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...} ] } ``` ### Get Single Shot History Request Type: req:history:get **Res...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ... Notes API Endpoints ### Get Shot Notes Request Type: req:history:notes:get ...
(QB_NEW_EN)
[grammar] ~117-~117: There might be a mistake here.
Context: ...692123456 } } ``` ### Save Shot Notes Request Type: req:history:notes:save...
(QB_NEW_EN)
[grammar] ~156-~156: There might be a mistake here.
Context: ... new ShotNotesCard component provides: - Star rating system (1-5 stars) - Dose in...
(QB_NEW_EN)
[grammar] ~157-~157: There might be a mistake here.
Context: ...ovides: - Star rating system (1-5 stars) - Dose in/out fields with automatic ratio ...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ... fields with automatic ratio calculation - Grind setting input - Balance/taste sele...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ... ratio calculation - Grind setting input - Balance/taste selector (bitter, balanced...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: .../taste selector (bitter, balanced, sour) - Free-form notes text area - Edit/save fu...
(QB_NEW_EN)
[grammar] ~161-~161: There might be a mistake here.
Context: ...anced, sour) - Free-form notes text area - Edit/save functionality The dose out fi...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...ced to automatically include notes data: - Export filename: `{shot-id}-complete.jso...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ... include notes data: - Export filename: {shot-id}-complete.json - Contains both shot history data and note...
(QB_NEW_EN)
[grammar] ~201-~201: There might be a mistake here.
Context: ...ma defined in /schema/shot_notes.json: - id: Shot ID (required) - rating: Star ra...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ..._notes.json: - id: Shot ID (required) - rating: Star rating 0-5 - doseIn`: Input dose...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...D (required) - rating: Star rating 0-5 - doseIn: Input dose in grams - doseOut: Outpu...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...ting 0-5 - doseIn: Input dose in grams - doseOut: Output dose in grams - ratio: Calcul...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ... grams - doseOut: Output dose in grams - ratio: Calculated ratio (doseOut/doseIn) - `g...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ...atio: Calculated ratio (doseOut/doseIn) - grindSetting`: String description of grind setting - ...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ...ng: String description of grind setting - balanceTaste`: One of "bitter", "balanced", "sour" - ...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...te: One of "bitter", "balanced", "sour" - notes: Free-form text notes - timestamp`: Wh...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ..., "sour" - notes: Free-form text notes - timestamp: When notes were last updated
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ...timestamp`: When notes were last updated
(QB_NEW_EN)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (2)
web/src/pages/ShotHistory/utils.js (1)
37-39: Notes passthrough LGTMConditionally attaching shot.notes to the parsed result is straightforward and works as intended.
src/display/plugins/ShotHistoryPlugin.cpp (1)
10-17: Good job replacing try/catch with explicit guardsThis aligns with embedded best practices and the PR feedback about exception handling.
dragm83
force-pushed
the
shot_record_fix
branch
from
107dbfb to
5fcfa07
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
src/display/plugins/ShotHistoryPlugin.h (1)
42-45: Fix printf format specifier for unsigned long timestampUsing %d for an unsigned long is undefined behavior on some targets.
Apply this diff:
- return string_format("%d,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f", t, tt, ct, tp, cp, fl, tf, pf, vf, v, + return string_format("%lu,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f", t, tt, ct, tp, cp, fl, tf, pf, vf, v, ev);src/display/plugins/ShotHistoryPlugin.cpp (4)
1-7: Include for fabsf/isfinite usageRequired for the floating-point checks and fabsf fix suggested below.
Apply this diff:
#include <display/core/utils.h> +#include <cmath>
18-31: Guard against non-finite weights and division by zero in flow calcAvoid NaNs/Infs and a possible divide-by-zero when two samples land in the same millisecond.
Apply this diff:
- // Validate weight data before processing - if (isnan(weight) || weight < 0) { + // Validate weight data before processing (embedded-safe) + if (!std::isfinite(weight) || weight < 0.0f) { return; // Skip invalid weight data } @@ - if (lastVolumeSample != 0) { - const unsigned long timeDiff = now - lastVolumeSample; - const float volumeDiff = weight - currentBluetoothWeight; - currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; - } + if (lastVolumeSample != 0) { + const unsigned long timeDiff = now - lastVolumeSample; + if (timeDiff > 0) { // avoid division by zero when multiple samples arrive within the same ms + const float volumeDiff = weight - currentBluetoothWeight; + currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; + } + }
54-55: Fix printf specifier for getTime()getTime() returns an unsigned long; use %lu to avoid UB.
Apply this diff:
- file.printf("1,%s,%ld\n", currentProfileName.c_str(), getTime()); + file.printf("1,%s,%lu\n", currentProfileName.c_str(), getTime());
185-200: Cleanup should retain MAX_HISTORY_ENTRIES shots (by ID), not filesCurrent logic counts files (.dat and .json), effectively halving capacity and desynchronizing pairs. Group by shot ID and remove both files together.
Apply this diff:
- File directory = SPIFFS.open("/h"); - std::vector<String> entries; - String filename = directory.getNextFileName(); - while (filename != "") { - entries.push_back(filename); - filename = directory.getNextFileName(); - } - sort(entries.begin(), entries.end(), [](String a, String b) { return a < b; }); - if (entries.size() > MAX_HISTORY_ENTRIES) { - for (unsigned int i = 0; i < entries.size() - MAX_HISTORY_ENTRIES; i++) { - String name = entries[i]; - SPIFFS.remove(name); - } - } + File directory = SPIFFS.open("/h"); + std::vector<String> ids; + String filename = directory.getNextFileName(); + while (filename != "") { + if (filename.endsWith(".dat")) { + int start = filename.lastIndexOf('/') + 1; + int end = filename.lastIndexOf('.'); + if (start >= 0 && end > start) { + ids.push_back(filename.substring(start, end)); + } + } + filename = directory.getNextFileName(); + } + std::sort(ids.begin(), ids.end(), [](const String &a, const String &b) { return a < b; }); + if (ids.size() > MAX_HISTORY_ENTRIES) { + const size_t toRemove = ids.size() - MAX_HISTORY_ENTRIES; + for (size_t i = 0; i < toRemove; i++) { + const String &id = ids[i]; + SPIFFS.remove("/h/" + id + ".dat"); + SPIFFS.remove("/h/" + id + ".json"); + } + }
♻️ Duplicate comments (8)
src/display/plugins/ShotHistoryPlugin.h (2)
27-28: ArduinoJson API misuse: avoid JsonDocument in interfaces; switch to JsonVariant-based APIThis will otherwise force callers to incorrectly allocate/construct JsonDocument (as seen in the .cpp), causing compile-time/runtime issues on ArduinoJson v6.
Apply this diff:
- void saveNotes(const String &id, const JsonDocument ¬es); - void loadNotes(const String &id, JsonDocument ¬es); + void saveNotes(const String &id, JsonVariantConst notes); + bool loadNotes(const String &id, JsonVariant dst);Follow up: update the .cpp implementation and all call sites accordingly (see comments in .cpp).
61-71: Shared state accessed across event callbacks and task: mark fields volatileThese scalars are written in event handlers and read in loopTask/record(); mark as volatile to avoid torn/non-visible reads on ESP32. Consider adding minimal critical sections if issues persist.
Apply this diff:
- bool recording = false; - bool headerWritten = false; - bool extendedRecording = false; + volatile bool recording = false; + volatile bool headerWritten = false; + volatile bool extendedRecording = false; @@ - unsigned long lastVolumeSample = 0; + volatile unsigned long lastVolumeSample = 0; @@ - unsigned long lastWeightChangeTime = 0; + volatile unsigned long lastWeightChangeTime = 0; @@ - float currentBluetoothWeight = 0.0f; + volatile float currentBluetoothWeight = 0.0f; @@ - float lastStableWeight = 0.0f; + volatile float lastStableWeight = 0.0f; @@ - float currentBluetoothFlow = 0.0f; + volatile float currentBluetoothFlow = 0.0f;src/display/plugins/ShotHistoryPlugin.cpp (6)
89-90: Use fabsf for float absolute differenceabs() may call the int overload; fabsf() is the correct choice for float.
Apply this diff:
- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);
223-227: Fix ArduinoJson handling: deserialize notes directly into response doc (no intermediate JsonDocument)Avoid constructing JsonDocument without capacity; load into response["notes"] and remove if missing.
Apply this diff:
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - o["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, o["notes"])) { + o.remove("notes"); + }
240-245: Same fix for single history response: avoid intermediate JsonDocumentLoad notes into response["notes"] directly.
Apply this diff:
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - response["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, response["notes"])) { + response.remove("notes"); + }
250-265: Notes endpoints: avoid constructing JsonDocument; validate input and set timestamp safelyUse JsonObject/JsonVariant and remove reliance on exception mechanics. This also satisfies the “no try/catch” guidance for embedded builds.
Apply this diff:
} else if (type == "req:history:delete") { String id = request["id"].as<String>(); SPIFFS.remove("/h/" + id + ".dat"); SPIFFS.remove("/h/" + id + ".json"); // Also remove notes file if it exists response["msg"] = "Ok"; } else if (type == "req:history:notes:get") { - String id = request["id"].as<String>(); - JsonDocument notes; - loadNotes(id, notes); - response["notes"] = notes; + String id = request["id"].as<String>(); + if (!loadNotes(id, response["notes"])) { + response["notes"] = JsonObject(); // empty object if not found + } } else if (type == "req:history:notes:save") { - String id = request["id"].as<String>(); - JsonDocument notes = request["notes"]; - notes["timestamp"] = getTime(); - saveNotes(id, notes); + String id = request["id"].as<String>(); + JsonObject notes = request["notes"].as<JsonObject>(); + if (notes.isNull()) { + response["error"] = "invalid notes"; + return; + } + notes["timestamp"] = getTime(); + saveNotes(id, notes); response["msg"] = "Ok";
268-286: Refactor saveNotes/loadNotes to JsonVariant-based API and stream to/from SPIFFSPrevents incorrect JsonDocument construction and reduces heap churn by streaming JSON.
Apply this diff:
-void ShotHistoryPlugin::saveNotes(const String &id, const JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); - if (file) { - String notesStr; - serializeJson(notes, notesStr); - file.print(notesStr); - file.close(); - } -} +void ShotHistoryPlugin::saveNotes(const String &id, JsonVariantConst notes) { + File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); + if (!file) return; + serializeJson(notes, file); + file.close(); +} -void ShotHistoryPlugin::loadNotes(const String &id, JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", "r"); - if (file) { - String notesStr = file.readString(); - file.close(); - deserializeJson(notes, notesStr); - } -} +bool ShotHistoryPlugin::loadNotes(const String &id, JsonVariant dst) { + File file = SPIFFS.open("/h/" + id + ".json", "r"); + if (!file) return false; + // Parse into a temporary document, then copy into dst to keep ownership in caller's document. + DynamicJsonDocument tmp(1024); + DeserializationError err = deserializeJson(tmp, file); + file.close(); + if (err) return false; + dst.set(tmp.as<JsonVariant>()); + return true; +}Note: Adjust tmp capacity (1024) if your notes can exceed ~1KB.
152-168: Trigger extended recording based on recent BLE samples (mode-agnostic)This aligns behavior with documentation (“regardless of volumetric or time-based mode”).
Apply this diff:
- // Check if we have active bluetooth weight data (regardless of volumetric mode) - bool hasActiveWeightData = false; - - if (controller != nullptr) { - if (controller->isVolumetricAvailable() && currentBluetoothWeight > 0) { - hasActiveWeightData = true; - } - } - - if (hasActiveWeightData) { + // Check if we have active bluetooth weight data (mode-agnostic) + const unsigned long now = millis(); + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + const bool hasActiveWeightData = hasRecentSample && (currentBluetoothWeight > 0.0f); + + if (hasActiveWeightData) {
🧹 Nitpick comments (3)
web/src/pages/ShotHistory/HistoryCard.jsx (2)
9-13: Use application/json MIME for export data URLMinor correctness/UX improvement for the exported file type.
Apply this diff:
- const dataStr = - 'data:text/json;charset=utf-8,' + encodeURIComponent(JSON.stringify(shot, undefined, 2)); + const dataStr = + 'data:application/json;charset=utf-8,' + encodeURIComponent(JSON.stringify(shot, undefined, 2));
27-33: Pass onExport directly to onClickNo need for the wrapper; this avoids creating a new function each render.
Apply this diff:
- onClick={() => onExport()} + onClick={onExport}docs/shot-notes-api.md (1)
11-19: Minor editorial/grammar fixes for clarity and consistencyPolish language (avoid contractions, improve concision) and match style.
Apply this diff:
-When brewing with a connected Bluetooth scale (regardless of volumetric or time-based mode), the system now continues recording for a brief period after the shot officially ends to capture the additional liquid that drips from the portafilter. +When brewing with a connected Bluetooth scale (regardless of volumetric or time-based mode), the system continues recording briefly after the shot officially ends to capture additional liquid that drips from the portafilter. @@ -- **Weight Stabilization**: Stops early if weight doesn't change by more than 0.1g for 1 second +- **Weight Stabilization**: Stops early if the weight does not change by more than 0.1 g for 1 second @@ -This ensures that the recorded shot data includes the complete extraction weight, not just the weight at the moment the shot timer stops. +This ensures the recorded shot data includes the complete extraction weight, not just the weight when the shot timer stops. @@ -- Export filename: `{shot-id}-complete.json` +- Export filename: `{shot-id}-complete.json` - Contains both shot history data and notes data in a single JSON file - Notes are automatically included if they exist for the shot - Maintains backward compatibility - shots without notes export normallyAlso applies to: 27-27, 168-173
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
docs/shot-notes-api.md(1 hunks)schema/shot_notes.json(1 hunks)src/display/plugins/ShotHistoryPlugin.cpp(7 hunks)src/display/plugins/ShotHistoryPlugin.h(4 hunks)web/src/pages/ShotHistory/HistoryCard.jsx(3 hunks)web/src/pages/ShotHistory/ShotNotesCard.jsx(1 hunks)web/src/pages/ShotHistory/utils.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- schema/shot_notes.json
- web/src/pages/ShotHistory/utils.js
- web/src/pages/ShotHistory/ShotNotesCard.jsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/display/plugins/ShotHistoryPlugin.cpp (1)
web/src/pages/ShotHistory/ShotNotesCard.jsx (2)
notes(7-16)saveNotes(61-78)
src/display/plugins/ShotHistoryPlugin.h (2)
web/src/pages/ShotHistory/ShotNotesCard.jsx (2)
saveNotes(61-78)notes(7-16)src/display/plugins/ShotHistoryPlugin.cpp (6)
saveNotes(268-276)saveNotes(268-268)loadNotes(278-285)loadNotes(278-278)finalizeRecording(174-183)finalizeRecording(174-174)
🪛 LanguageTool
docs/shot-notes-api.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...ortafilter. Extended Recording Logic: - Trigger: Automatically activates when ...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...if weight doesn't change by more than 0.1g for 1 second - Purpose: Captures mo...
(QB_NEW_EN)
[style] ~27-~27: For conciseness, consider replacing this expression with an adverb.
Context: ... extraction weight, not just the weight at the moment the shot timer stops. ## Enhanced Shot...
(AT_THE_MOMENT)
[grammar] ~33-~33: There might be a mistake here.
Context: ...en available. ### Get Shot History List Request Type: req:history:list **Re...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...} ] } ``` ### Get Single Shot History Request Type: req:history:get **Res...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ... Notes API Endpoints ### Get Shot Notes Request Type: req:history:notes:get ...
(QB_NEW_EN)
[grammar] ~117-~117: There might be a mistake here.
Context: ...692123456 } } ``` ### Save Shot Notes Request Type: req:history:notes:save...
(QB_NEW_EN)
[grammar] ~156-~156: There might be a mistake here.
Context: ... new ShotNotesCard component provides: - Star rating system (1-5 stars) - Dose in...
(QB_NEW_EN)
[grammar] ~157-~157: There might be a mistake here.
Context: ...ovides: - Star rating system (1-5 stars) - Dose in/out fields with automatic ratio ...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ... fields with automatic ratio calculation - Grind setting input - Balance/taste sele...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ... ratio calculation - Grind setting input - Balance/taste selector (bitter, balanced...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: .../taste selector (bitter, balanced, sour) - Free-form notes text area - Edit/save fu...
(QB_NEW_EN)
[grammar] ~161-~161: There might be a mistake here.
Context: ...anced, sour) - Free-form notes text area - Edit/save functionality The dose out fi...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...ced to automatically include notes data: - Export filename: `{shot-id}-complete.jso...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ... include notes data: - Export filename: {shot-id}-complete.json - Contains both shot history data and note...
(QB_NEW_EN)
[grammar] ~201-~201: There might be a mistake here.
Context: ...ma defined in /schema/shot_notes.json: - id: Shot ID (required) - rating: Star ra...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ..._notes.json: - id: Shot ID (required) - rating: Star rating 0-5 - doseIn`: Input dose...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...D (required) - rating: Star rating 0-5 - doseIn: Input dose in grams - doseOut: Outpu...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...ting 0-5 - doseIn: Input dose in grams - doseOut: Output dose in grams - ratio: Calcul...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ... grams - doseOut: Output dose in grams - ratio: Calculated ratio (doseOut/doseIn) - `g...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ...atio: Calculated ratio (doseOut/doseIn) - grindSetting`: String description of grind setting - ...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ...ng: String description of grind setting - balanceTaste`: One of "bitter", "balanced", "sour" - ...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...te: One of "bitter", "balanced", "sour" - notes: Free-form text notes - timestamp`: Wh...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ..., "sour" - notes: Free-form text notes - timestamp: When notes were last updated
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ...timestamp`: When notes were last updated
(QB_NEW_EN)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (5)
web/src/pages/ShotHistory/HistoryCard.jsx (2)
4-4: ShotNotesCard integration looks goodImporting and rendering ShotNotesCard with the shot prop is correct and aligns with the new notes flow.
Also applies to: 61-61
13-13: Export filename and useCallback dependency update LGTMThe “-complete.json” filename clarifies the payload contents, and adding [shot] as a dependency avoids stale closures.
Also applies to: 17-17
src/display/plugins/ShotHistoryPlugin.h (1)
10-13: New recording and stabilization constants are appropriateThe chosen timeouts/thresholds are reasonable defaults for post-shot drip capture.
src/display/plugins/ShotHistoryPlugin.cpp (2)
174-183: Finalize recording and notes cleanup LGTMDeleting the notes file alongside short/invalid shots is consistent with the data model.
18-24: Kudos: exception-free guards align with embedded best practicesThanks for removing try/catch and switching to explicit guards; this adheres to the “no exceptions” guidance for embedded builds.
Also applies to: 72-88, 152-168
| // Explicit safety checks instead of try/catch for embedded systems | ||
| bool canProcessWeight = (controller != nullptr); | ||
| if (canProcessWeight) { | ||
| canProcessWeight = controller->isVolumetricAvailable(); | ||
| } | ||
|
|
||
| if (!canProcessWeight) { | ||
| // If BLE connection is unstable, end extended recording early | ||
| extendedRecording = false; | ||
| finalizeRecording(); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extended-recording gating should rely on recent weight samples, not volumetric availability
This ensures post-brew drip is captured even when volumetric mode is unavailable, matching the PR objective and docs.
Apply this diff:
- // Explicit safety checks instead of try/catch for embedded systems
- bool canProcessWeight = (controller != nullptr);
- if (canProcessWeight) {
- canProcessWeight = controller->isVolumetricAvailable();
- }
-
- if (!canProcessWeight) {
- // If BLE connection is unstable, end extended recording early
- extendedRecording = false;
- finalizeRecording();
- return;
- }
+ // Consider BLE weight active if we've seen a recent sample
+ const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000);
+ if (!hasRecentSample) {
+ // No recent weight sample, end extended recording early
+ extendedRecording = false;
+ finalizeRecording();
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/display/plugins/ShotHistoryPlugin.cpp around lines 76 to 88, the gating
for extendedRecording currently aborts when controller->isVolumetricAvailable()
is false; change it to instead check recent weight samples for post-brew drip
activity. Replace the volumetric availability gate with a small routine that
queries the weight-sensor buffer (e.g., last N samples or samples within the
last T seconds) and determines if recent weight changes exceed a configured
threshold; only set extendedRecording = false and call finalizeRecording() when
there are no recent weight-activity samples indicating drip. Keep the volumetric
availability check separate (do not use it to block extended recording), and
ensure you handle nullptr controller and empty sample buffers safely.
38ad7ea to
c0efbee
Compare
dragm83
force-pushed
the
shot_record_fix
branch
from
c0efbee to
bbdbbf5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/display/plugins/ShotHistoryPlugin.cpp (4)
17-42: Remove duplicate BLE handler body and avoid exceptions; add guardsThe BLE handler contains two nearly identical blocks; one is wrapped in try/catch. Embedded builds often disable exceptions; drop try/catch and keep one guarded path with finite/zero-division protection.
Apply this diff:
- pm->on("controller:volumetric-measurement:bluetooth:change", [this](Event const &event) { - const float weight = event.getFloat("value"); - const unsigned long now = millis(); - if (lastVolumeSample != 0) { - const unsigned long timeDiff = now - lastVolumeSample; - const float volumeDiff = weight - currentBluetoothWeight; - const float volumeFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; - currentBluetoothFlow = currentBluetoothFlow * 0.9f + volumeFlow * 0.1f; - } - lastVolumeSample = now; - currentBluetoothWeight = weight; - try { - const float weight = event.getFloat("value"); - const unsigned long now = millis(); - if (lastVolumeSample != 0) { - const unsigned long timeDiff = now - lastVolumeSample; - const float volumeDiff = weight - currentBluetoothWeight; - currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; - } - lastVolumeSample = now; - currentBluetoothWeight = weight; - } catch (...) { - // If there's any exception processing weight data, ignore this update - // This prevents crashes if BLE data is corrupted or connection is unstable - } - }); + pm->on("controller:volumetric-measurement:bluetooth:change", [this](Event const &event) { + const float weight = event.getFloat("value"); + if (!std::isfinite(weight)) return; + const unsigned long now = millis(); + if (lastVolumeSample != 0) { + const unsigned long timeDiff = now - lastVolumeSample; + if (timeDiff == 0) return; // avoid div-by-zero on same-tick samples + const float volumeDiff = weight - currentBluetoothWeight; + const float volumeFlow = (volumeDiff / static_cast<float>(timeDiff)) * 1000.0f; + currentBluetoothFlow = currentBluetoothFlow * 0.9f + volumeFlow * 0.1f; + } + lastVolumeSample = now; + currentBluetoothWeight = weight; + });
122-132: Unify “minimum valid shot duration” and delete both files consistentlyTwo different thresholds exist (7.5 s vs. 5 s), and only one branch removes the notes file. Define a single constant and apply consistently.
Apply this diff to define a constant near the includes (or in the header), then use it in both places:
+// Minimum shot duration to keep history (exclude flush/failed shots) +constexpr unsigned long MIN_VALID_SHOT_DURATION = 5000; // 5 seconds- unsigned long duration = millis() - shotStart; - if (duration <= 7500) { // Exclude failed shots and flushes - SPIFFS.remove("/h/" + currentId + ".dat"); - } else { + unsigned long duration = millis() - shotStart; + if (duration <= MIN_VALID_SHOT_DURATION) { + SPIFFS.remove("/h/" + currentId + ".dat"); + SPIFFS.remove("/h/" + currentId + ".json"); + } else { controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); cleanupHistory(); }- unsigned long duration = millis() - shotStart; - if (duration <= 5000) { + unsigned long duration = millis() - shotStart; + if (duration <= MIN_VALID_SHOT_DURATION) { SPIFFS.remove("/h/" + currentId + ".dat"); SPIFFS.remove("/h/" + currentId + ".json"); // Also remove notes file if it exists } else { controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); cleanupHistory(); }Also applies to: 190-197
200-215: Cleanup counts files, not shots; may over-delete with new .json filesBecause both “.dat” and “.json” are listed, the count can exceed
MAX_HISTORY_ENTRIESby 2x and delete too many items. Build the list from “.dat” only and delete both files per shot ID.Apply this diff:
- std::vector<String> entries; + std::vector<String> shotIds; String filename = directory.getNextFileName(); while (filename != "") { - entries.push_back(filename); + if (filename.endsWith(".dat")) { + int start = filename.lastIndexOf('/') + 1; + int end = filename.lastIndexOf('.'); + shotIds.push_back(filename.substring(start, end)); + } filename = directory.getNextFileName(); } - sort(entries.begin(), entries.end(), [](String a, String b) { return a < b; }); - if (entries.size() > MAX_HISTORY_ENTRIES) { - for (unsigned int i = 0; i < entries.size() - MAX_HISTORY_ENTRIES; i++) { - String name = entries[i]; - SPIFFS.remove(name); - } + sort(shotIds.begin(), shotIds.end(), [](String a, String b) { return a < b; }); + if (shotIds.size() > MAX_HISTORY_ENTRIES) { + const size_t toRemove = shotIds.size() - MAX_HISTORY_ENTRIES; + for (size_t i = 0; i < toRemove; i++) { + const String &id = shotIds[i]; + SPIFFS.remove("/h/" + id + ".dat"); + SPIFFS.remove("/h/" + id + ".json"); + } }
1-309: Remove remaining try/catch blocks for embedded safetyThe embedded build still includes a
try { … } catch (...) { … }around the Bluetooth weight handling, which must be refactored to explicit error-checking.• File
src/display/plugins/ShotHistoryPlugin.cpp, around lines 28–38:try { const float weight = event.getFloat("value"); const unsigned long now = millis(); if (lastVolumeSample != 0) { const unsigned long timeDiff = now - lastVolumeSample; const float volumeDiff = weight - currentBluetoothWeight; currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; } lastVolumeSample = now; currentBluetoothWeight = weight; } catch (...) { // If there's any exception processing weight data, ignore this update }Replace this with pre-validation (e.g., checking
eventpayload integrity or return-code APIs) instead of relying on C++ exceptions.
♻️ Duplicate comments (8)
web/src/pages/ShotHistory/ShotNotesCard.jsx (1)
21-27: Guard ratio against divide-by-zero/NaN (repeat)Reiterating prior feedback: if doseIn is "0" or non-numeric, ratio becomes Infinity/NaN. Add validation before computing.
Apply this diff:
- useEffect(() => { - if (notes.doseIn && notes.doseOut) { - const ratio = (parseFloat(notes.doseOut) / parseFloat(notes.doseIn)).toFixed(2); - setNotes(prev => ({ ...prev, ratio })); - } - }, [notes.doseIn, notes.doseOut]); + useEffect(() => { + const inVal = parseFloat(notes.doseIn); + const outVal = parseFloat(notes.doseOut); + if (!Number.isNaN(inVal) && !Number.isNaN(outVal) && inVal > 0) { + const ratio = (outVal / inVal).toFixed(2); + setNotes(prev => ({ ...prev, ratio })); + } else { + setNotes(prev => ({ ...prev, ratio: '' })); + } + }, [notes.doseIn, notes.doseOut]);src/display/plugins/ShotHistoryPlugin.cpp (7)
283-299: Refactor saveNotes/loadNotes to use JsonVariant and stream I/OAvoid intermediate
Stringbuffers andJsonDocumentcopies; deserialize directly to destination.Apply this diff:
-void ShotHistoryPlugin::saveNotes(const String &id, const JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); - if (file) { - String notesStr; - serializeJson(notes, notesStr); - file.print(notesStr); - file.close(); - } -} +void ShotHistoryPlugin::saveNotes(const String &id, JsonVariantConst notes) { + File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); + if (!file) return; + serializeJson(notes, file); + file.close(); +} -void ShotHistoryPlugin::loadNotes(const String &id, JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", "r"); - if (file) { - String notesStr = file.readString(); - file.close(); - deserializeJson(notes, notesStr); - } -} +bool ShotHistoryPlugin::loadNotes(const String &id, JsonVariant dst) { + File file = SPIFFS.open("/h/" + id + ".json", "r"); + if (!file) return false; + DeserializationError err = deserializeJson(dst, file); + file.close(); + return !err; +}Note: adjust declarations in the header accordingly (
bool loadNotes(const String&, JsonVariant);andvoid saveNotes(const String&, JsonVariantConst);).
237-242: Deserialize notes directly into response; avoid extra JsonDocument (ArduinoJson v6)Copying via a separate
JsonDocumentcan fragment memory. Load intoo["notes"]directly.Apply this diff:
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - o["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, o["notes"])) { + o.remove("notes"); + }
255-261: Same: load notes into response document in-placeAvoid allocating another
JsonDocument.Apply this diff:
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - response["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, response["notes"])) { + response.remove("notes"); + }
269-280: Notes get/save: avoid constructing JsonDocument; validate and set timestamp safelyOperate on JsonObject/JsonVariant to reduce heap usage and guard inputs.
Apply this diff:
- } else if (type == "req:history:notes:get") { - String id = request["id"].as<String>(); - JsonDocument notes; - loadNotes(id, notes); - response["notes"] = notes; - } else if (type == "req:history:notes:save") { - String id = request["id"].as<String>(); - JsonDocument notes = request["notes"]; - notes["timestamp"] = getTime(); - saveNotes(id, notes); + } else if (type == "req:history:notes:get") { + String id = request["id"].as<String>(); + if (!loadNotes(id, response["notes"])) { + response["notes"] = JsonObject(); // empty object if not found + } + } else if (type == "req:history:notes:save") { + String id = request["id"].as<String>(); + JsonObject notes = request["notes"].as<JsonObject>(); + if (notes.isNull()) { + response["error"] = "invalid notes"; + return; + } + notes["timestamp"] = getTime(); + saveNotes(id, notes); response["msg"] = "Ok";
97-112: Use floating-point absolute; avoid abs(int) overload
abscan select the integer overload. Usefabsf.Apply this diff:
- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);
84-95: Don’t gate extended recording on volumetric availabilityThis contradicts the PR objective/docs (“mode independent”). Use recency of BLE samples instead.
Apply this diff:
- // Explicit safety checks instead of try/catch for embedded systems - bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } - - if (!canProcessWeight) { + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= WEIGHT_STABILIZATION_TIME); + if (!hasRecentSample) { // If BLE connection is unstable, end extended recording early extendedRecording = false; finalizeRecording(); return; }
167-183: Start extended recording based on recent BLE data, not volumetric availabilityAligns with “mode independent” behavior and handles cases where volumetric is off but scale is connected.
Apply this diff:
- // Check if we have active bluetooth weight data (regardless of volumetric mode) - bool hasActiveWeightData = false; - - if (controller != nullptr) { - if (controller->isVolumetricAvailable() && currentBluetoothWeight > 0) { - hasActiveWeightData = true; - } - } + // Start extended recording if we have recent BLE weight data + bool hasActiveWeightData = false; + const unsigned long now = millis(); + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= WEIGHT_STABILIZATION_TIME); + if (hasRecentSample && currentBluetoothWeight > 0.0f) { + hasActiveWeightData = true; + }
🧹 Nitpick comments (8)
docs/shot-notes-api.md (2)
164-165: Use consistent terminology: it’s weight (g), not volume“Dose out field is automatically pre-populated with the final volume measurement…” — elsewhere, doses are expressed in grams and the stabilization uses grams. Suggest replacing “volume measurement” with “weight (g)” for consistency and to avoid confusion.
Apply this diff:
- The dose out field is automatically pre-populated with the final volume measurement from the shot data. + The dose out field is automatically pre-populated with the final weight (g) from the shot data.
13-18: Tighten phrasing of the extended-recording bulletsMinor clarity/grammar nits.
Apply this diff:
-**Extended Recording Logic:** -- **Trigger**: Automatically activates when Bluetooth scale data is available and weight > 0 -- **Mode Independent**: Works for both volumetric and time-based shots -- **Duration**: Maximum 3 seconds additional recording -- **Weight Stabilization**: Stops early if weight doesn't change by more than 0.1g for 1 second -- **Purpose**: Captures more accurate final extraction weight +**Extended recording logic** +- **Trigger**: Activates when Bluetooth-scale data is available and weight > 0. +- **Mode independent**: Works for both volumetric and time-based shots. +- **Duration**: Up to 3 seconds of additional recording. +- **Weight stabilization**: Stops early if weight changes by < 0.1 g over 1 second. +- **Purpose**: Capture a more accurate final extraction weight.web/src/pages/ShotHistory/ShotNotesCard.jsx (4)
37-48: Ensure loaded notes cannot override the shot IDIf a stale/incorrect notes file has a mismatched
id, your merge will silently adopt it. Forceidtoshot.idwhen loading/merging.Apply this diff:
- const loadedNotes = { ...response.notes }; + const loadedNotes = { ...response.notes, id: shot.id }; if (!loadedNotes.doseOut && shot.volume) { loadedNotes.doseOut = shot.volume.toFixed(1); } setNotes(prev => ({ ...prev, ...loadedNotes }));
169-176: Inputs: add min=0 and better mobile keypad hintsDose values shouldn’t be negative; add
min="0"andinputMode="decimal"to improve UX and prevent invalid inputs.Apply this diff:
- <input - type="number" - step="0.1" + <input + type="number" + step="0.1" + min="0" + inputMode="decimal" className="input input-bordered w-full" value={notes.doseIn} onChange={(e) => handleInputChange('doseIn', e.target.value)} placeholder="18.0" />- <input - type="number" - step="0.1" + <input + type="number" + step="0.1" + min="0" + inputMode="decimal" className="input input-bordered w-full" value={notes.doseOut} onChange={(e) => handleInputChange('doseOut', e.target.value)} placeholder="36.0" />Also applies to: 190-197
88-99: Improve accessibility of rating buttonsProvide accessible labels and roles for the star buttons.
Apply this diff:
- <button + <button key={i} type="button" disabled={!editable} onClick={() => editable && handleInputChange('rating', i)} + aria-label={`Set rating to ${i} star${i > 1 ? 's' : ''}`} + role="radio" + aria-checked={i === rating} className={`text-lg ${i <= rating ? 'text-yellow-400' : 'text-gray-300'} ${ editable ? 'hover:text-yellow-300 cursor-pointer' : 'cursor-default' }`} > ★ </button>
126-134: Cancel currently doesn’t revert editsCancel exits edit mode but keeps unsaved changes. If that’s unintended, keep a
draftNotesstate while editing and discard it on cancel.Would you like me to provide a diff that introduces
draftNotesfor true cancel semantics?src/display/plugins/ShotHistoryPlugin.cpp (2)
3-7: Include for finite/abs checksYou’ll need
<cmath>forstd::isfinite/fabsf.Apply this diff:
#include <SPIFFS.h> #include <display/core/Controller.h> #include <display/core/ProfileManager.h> #include <display/core/utils.h> +#include <cmath>
21-24: Flow calc: also guard against zero time deltaYou skip only when
lastVolumeSample == 0, but same-tick updates can yieldtimeDiff == 0. Already included in my handler refactor; ensure it’s retained.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
docs/shot-notes-api.md(1 hunks)schema/shot_notes.json(1 hunks)src/display/plugins/ShotHistoryPlugin.cpp(6 hunks)src/display/plugins/ShotHistoryPlugin.h(4 hunks)web/src/pages/ShotHistory/HistoryCard.jsx(3 hunks)web/src/pages/ShotHistory/ShotNotesCard.jsx(1 hunks)web/src/pages/ShotHistory/utils.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- schema/shot_notes.json
- web/src/pages/ShotHistory/utils.js
- web/src/pages/ShotHistory/HistoryCard.jsx
- src/display/plugins/ShotHistoryPlugin.h
🧰 Additional context used
🧬 Code Graph Analysis (2)
web/src/pages/ShotHistory/ShotNotesCard.jsx (1)
src/display/plugins/ShotHistoryPlugin.cpp (4)
loadNotes(293-300)loadNotes(293-293)saveNotes(283-291)saveNotes(283-283)
src/display/plugins/ShotHistoryPlugin.cpp (1)
web/src/pages/ShotHistory/ShotNotesCard.jsx (2)
notes(7-16)saveNotes(61-78)
🪛 LanguageTool
docs/shot-notes-api.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...ortafilter. Extended Recording Logic: - Trigger: Automatically activates when ...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...if weight doesn't change by more than 0.1g for 1 second - Purpose: Captures mo...
(QB_NEW_EN)
[style] ~27-~27: For conciseness, consider replacing this expression with an adverb.
Context: ... extraction weight, not just the weight at the moment the shot timer stops. ## Enhanced Shot...
(AT_THE_MOMENT)
[grammar] ~33-~33: There might be a mistake here.
Context: ...en available. ### Get Shot History List Request Type: req:history:list **Re...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...} ] } ``` ### Get Single Shot History Request Type: req:history:get **Res...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ... Notes API Endpoints ### Get Shot Notes Request Type: req:history:notes:get ...
(QB_NEW_EN)
[grammar] ~117-~117: There might be a mistake here.
Context: ...692123456 } } ``` ### Save Shot Notes Request Type: req:history:notes:save...
(QB_NEW_EN)
[grammar] ~156-~156: There might be a mistake here.
Context: ... new ShotNotesCard component provides: - Star rating system (1-5 stars) - Dose in...
(QB_NEW_EN)
[grammar] ~157-~157: There might be a mistake here.
Context: ...ovides: - Star rating system (1-5 stars) - Dose in/out fields with automatic ratio ...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ... fields with automatic ratio calculation - Grind setting input - Balance/taste sele...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ... ratio calculation - Grind setting input - Balance/taste selector (bitter, balanced...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: .../taste selector (bitter, balanced, sour) - Free-form notes text area - Edit/save fu...
(QB_NEW_EN)
[grammar] ~161-~161: There might be a mistake here.
Context: ...anced, sour) - Free-form notes text area - Edit/save functionality The dose out fi...
(QB_NEW_EN)
[grammar] ~168-~168: There might be a mistake here.
Context: ...ced to automatically include notes data: - Export filename: `{shot-id}-complete.jso...
(QB_NEW_EN)
[grammar] ~169-~169: There might be a mistake here.
Context: ... include notes data: - Export filename: {shot-id}-complete.json - Contains both shot history data and note...
(QB_NEW_EN)
[grammar] ~201-~201: There might be a mistake here.
Context: ...ma defined in /schema/shot_notes.json: - id: Shot ID (required) - rating: Star ra...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ..._notes.json: - id: Shot ID (required) - rating: Star rating 0-5 - doseIn`: Input dose...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...D (required) - rating: Star rating 0-5 - doseIn: Input dose in grams - doseOut: Outpu...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...ting 0-5 - doseIn: Input dose in grams - doseOut: Output dose in grams - ratio: Calcul...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ... grams - doseOut: Output dose in grams - ratio: Calculated ratio (doseOut/doseIn) - `g...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ...atio: Calculated ratio (doseOut/doseIn) - grindSetting`: String description of grind setting - ...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ...ng: String description of grind setting - balanceTaste`: One of "bitter", "balanced", "sour" - ...
(QB_NEW_EN)
[grammar] ~208-~208: There might be a mistake here.
Context: ...te: One of "bitter", "balanced", "sour" - notes: Free-form text notes - timestamp`: Wh...
(QB_NEW_EN)
[grammar] ~209-~209: There might be a mistake here.
Context: ..., "sour" - notes: Free-form text notes - timestamp: When notes were last updated
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ...timestamp`: When notes were last updated
(QB_NEW_EN)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (1)
docs/shot-notes-api.md (1)
11-19: Docs vs. implementation mismatch: “mode independent” but code gates on volumetric availabilityDocs state extended recording works regardless of mode, but the implementation still checks
controller->isVolumetricAvailable()before entering/continuing extended recording (see ShotHistoryPlugin.cpp Lines 84-95 and 170-174). Either update docs to reflect the gate, or (preferred) remove the volumetric-availability dependency in code so weight-based extended recording is truly mode independent.Do you want me to push a code change to rely on “recent BLE weight samples” instead (suggested in my C++ review)?
dragm83
force-pushed
the
shot_record_fix
branch
6 times, most recently
from
739fc70 to
247ea8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/display/plugins/ShotHistoryPlugin.cpp (1)
112-122: Bug: finalizeRecording and record() tail both update history — double increment riskWhen extendedRecording ends, you call finalizeRecording() (which increments history/cleanup) and return. On the next loop, the tail block runs, closes the file and may increment history again. This can bump history index twice and/or attempt to remove an already-deleted file.
Minimal, low-risk fix: introduce a finalized flag to guard the tail, set it in finalizeRecording(), and reset it in startRecording().
Header (already suggested):
+ bool finalized = false;Start of startRecording():
shotStart = millis(); + finalized = false;Set flag in finalizeRecording():
void ShotHistoryPlugin::finalizeRecording() { + finalized = true; unsigned long duration = millis() - shotStart;Guard the tail:
- if (!recording && !extendedRecording && isFileOpen) { + if (!recording && !extendedRecording && isFileOpen) { file.close(); isFileOpen = false; - unsigned long duration = millis() - shotStart; - if (duration <= 7500) { // Exclude failed shots and flushes - SPIFFS.remove("/h/" + currentId + ".dat"); - } else { - controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); - cleanupHistory(); - } + if (!finalized) { + unsigned long duration = millis() - shotStart; + if (duration <= 7500) { // Exclude failed shots and flushes + SPIFFS.remove("/h/" + currentId + ".dat"); + } else { + controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); + cleanupHistory(); + } + } }Alternative (cleaner but larger change): move File handling into a member and make finalizeRecording() close the file itself; then replace the tail block with a call to finalizeRecording(). The guard approach above keeps your current structure intact.
♻️ Duplicate comments (4)
src/display/plugins/ShotHistoryPlugin.h (1)
61-69: Data races on fields updated in event callbacks and read in loopTask/record()On ESP32 (dual-core FreeRTOS), these scalar fields are shared across contexts. At minimum mark them volatile; better, wrap multi-field reads/writes in a short critical section.
Apply this minimal diff to mark the hot-path scalars volatile:
- bool recording = false; + volatile bool recording = false; bool headerWritten = false; - bool extendedRecording = false; + volatile bool extendedRecording = false; unsigned long shotStart = 0; - unsigned long extendedRecordingStart = 0; - unsigned long lastVolumeSample = 0; - unsigned long lastWeightChangeTime = 0; + volatile unsigned long extendedRecordingStart = 0; + volatile unsigned long lastVolumeSample = 0; + volatile unsigned long lastWeightChangeTime = 0; float currentTemperature = 0.0f; - float currentBluetoothWeight = 0.0f; - float lastStableWeight = 0.0f; - float currentBluetoothFlow = 0.0f; + volatile float currentBluetoothWeight = 0.0f; + volatile float lastStableWeight = 0.0f; + volatile float currentBluetoothFlow = 0.0f;Optional (cleaner, prevents double finalization later): add a finalized flag:
+ bool finalized = false;Then reset it in startRecording() and set it in finalizeRecording() (see .cpp comments).
src/display/plugins/ShotHistoryPlugin.cpp (3)
71-85: Don’t gate extended recording on controller->isVolumetricAvailable(); rely on recent BLE weight samplesTying this to “volumetric available” can suppress valid post-brew drip capture. Instead, consider recent BLE sample presence within a time window (e.g., 1s), matching the PR objective.
- bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } - - if (!canProcessWeight) { + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= WEIGHT_STABILIZATION_TIME); + if (!hasRecentSample) { // If BLE connection is unstable, end extended recording early extendedRecording = false; finalizeRecording(); return; }
150-165: Start extended recording based on recent BLE samples, not volumetric availabilityMirrors the earlier gating fix so post-brew drip is captured even if volumetric mode reports unavailable.
- if (controller && - controller->isVolumetricAvailable() && - currentBluetoothWeight > 0) { + const unsigned long now = millis(); + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= WEIGHT_STABILIZATION_TIME); + if (hasRecentSample && currentBluetoothWeight > 0.0f) { // Start extended recording for any shot with active weight data extendedRecording = true; extendedRecordingStart = millis(); lastStableWeight = currentBluetoothWeight; lastWeightChangeTime = 0; return; // Don't finalize the recording yet }
87-102: Use fabsf for float absolute to avoid wrong overload and precision surprisesabs(...) may bind to the integer overload; use fabsf (or std::fabs).
- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);Note: The include <math.h> added above covers fabsf.
🧹 Nitpick comments (3)
src/display/plugins/ShotHistoryPlugin.h (1)
10-13: Constants align with PR goals; consider making user-configurable laterValues match “3s or until <0.1g delta for 1s.” If you anticipate machine variation, consider making them settings-backed (with sane defaults) rather than compile-time constants.
src/display/plugins/ShotHistoryPlugin.cpp (2)
132-141: Reset finalize guard during startRecordingIf you adopt the finalized guard, ensure it’s reset when a new recording starts.
lastVolumeSample = 0; lastWeightChangeTime = 0; extendedRecordingStart = 0; currentBluetoothWeight = 0.0f; lastStableWeight = 0.0f; currentEstimatedWeight = 0.0f; currentBluetoothFlow = 0.0f; currentProfileName = controller->getProfileManager()->getSelectedProfile().label; recording = true; extendedRecording = false; headerWritten = false; + finalized = false;
233-239: Use SHOT_HISTORY_INTERVAL for loop cadenceYou define SHOT_HISTORY_INTERVAL=100ms but loop uses a hardcoded 250ms. Use the constant for consistency and future tuning.
- vTaskDelay(250 / portTICK_PERIOD_MS); + vTaskDelay(SHOT_HISTORY_INTERVAL / portTICK_PERIOD_MS);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
schema/shot_notes.json(1 hunks)src/display/plugins/ShotHistoryPlugin.cpp(4 hunks)src/display/plugins/ShotHistoryPlugin.h(3 hunks)web/src/pages/ShotHistory/HistoryCard.jsx(2 hunks)web/src/pages/ShotHistory/ShotNotesCard.jsx(1 hunks)web/src/pages/ShotHistory/utils.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/pages/ShotHistory/HistoryCard.jsx
- web/src/pages/ShotHistory/utils.js
- web/src/pages/ShotHistory/ShotNotesCard.jsx
- schema/shot_notes.json
⏰ 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). (2)
- GitHub Check: deploy
- GitHub Check: test
🔇 Additional comments (1)
src/display/plugins/ShotHistoryPlugin.h (1)
51-51: Public finalizeRecording() is fine, but watch for duplication with record() tailGiven finalizeRecording() also advances history/cleanup, ensure record()’s tail doesn’t repeat those actions post-finalize. See my comment on the .cpp bottom block; a simple “finalized” guard can prevent double-increment.
| // Explicit check: Only process if weight is a valid number (not NaN or Inf) | ||
| if (!isnan(weight) && isfinite(weight)) { | ||
| if (lastVolumeSample != 0) { | ||
| const unsigned long timeDiff = now - lastVolumeSample; | ||
| const float volumeDiff = weight - currentBluetoothWeight; | ||
| currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; | ||
| } | ||
| lastVolumeSample = now; | ||
| currentBluetoothWeight = weight; | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard divide-by-zero and include proper math header for isnan/isfinite
Two fixes:
- timeDiff can be 0 if two BLE updates arrive in the same millisecond; guard before dividing.
- Include <math.h> (or ) explicitly to ensure isnan/isfinite are available across toolchains.
#include <display/core/utils.h>
+#include <math.h> // for isnan/isfinite/fabsf
...
- if (!isnan(weight) && isfinite(weight)) {
+ if (!isnan(weight) && isfinite(weight)) {
if (lastVolumeSample != 0) {
const unsigned long timeDiff = now - lastVolumeSample;
- const float volumeDiff = weight - currentBluetoothWeight;
- currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f;
+ if (timeDiff > 0) {
+ const float volumeDiff = weight - currentBluetoothWeight;
+ currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f;
+ }
}
lastVolumeSample = now;
currentBluetoothWeight = weight;📝 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.
| // Explicit check: Only process if weight is a valid number (not NaN or Inf) | |
| if (!isnan(weight) && isfinite(weight)) { | |
| if (lastVolumeSample != 0) { | |
| const unsigned long timeDiff = now - lastVolumeSample; | |
| const float volumeDiff = weight - currentBluetoothWeight; | |
| currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; | |
| } | |
| lastVolumeSample = now; | |
| currentBluetoothWeight = weight; | |
| } else { | |
| // At the top of the file, after the utils include | |
| #include <display/core/utils.h> | |
| #include <math.h> // for isnan/isfinite | |
| // …later, inside the pm->on handler around lines 20–29… | |
| // Explicit check: Only process if weight is a valid number (not NaN or Inf) | |
| if (!isnan(weight) && isfinite(weight)) { | |
| if (lastVolumeSample != 0) { | |
| const unsigned long timeDiff = now - lastVolumeSample; | |
| if (timeDiff > 0) { | |
| const float volumeDiff = weight - currentBluetoothWeight; | |
| currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; | |
| } | |
| } | |
| lastVolumeSample = now; | |
| currentBluetoothWeight = weight; | |
| } else { | |
| // existing else-branch… | |
| } |
🤖 Prompt for AI Agents
In src/display/plugins/ShotHistoryPlugin.cpp around lines 20 to 29, guard
against a potential divide-by-zero when computing currentBluetoothFlow by
checking that timeDiff is greater than zero before performing the division (skip
or set flow to 0 when timeDiff == 0), and add an explicit include for the math
header (<cmath> or <math.h>) at the top of the file so isnan and isfinite are
available across toolchains.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/display/plugins/ShotHistoryPlugin.cpp (2)
1-7: Include proper math header for isnan/isfinite/fabsf portability
isnan/isfinite/fabsfneed<math.h>(or<cmath>). Add it to avoid toolchain-dependent implicit declarations.#include <display/core/utils.h> +#include <math.h> // isnan, isfinite, fabsf
170-175: Retention may delete notes (.json) or non-history files; filter by shot IDs and remove paired files
cleanupHistory()currently sorts all entries and removes the oldest N regardless of extension. If notes are stored alongside as/h/{id}.json, they may be deleted prematurely or orphaned. Retain by shot ID (based on.dat) and delete both.datand.jsononly for pruned shots.- sort(entries.begin(), entries.end(), [](String a, String b) { return a < b; }); - if (entries.size() > MAX_HISTORY_ENTRIES) { - for (unsigned int i = 0; i < entries.size() - MAX_HISTORY_ENTRIES; i++) { - String name = entries[i]; - SPIFFS.remove(name); - } - } + // Build an ordered list of shot IDs from .dat files only + std::vector<String> shotIds; + for (auto const &name : entries) { + if (name.ends_with(".dat")) { + int start = name.lastIndexOf('/') + 1; + int end = name.lastIndexOf('.'); + if (start > 0 && end > start) { + shotIds.push_back(name.substring(start, end)); + } + } + } + sort(shotIds.begin(), shotIds.end(), [](String a, String b) { return a < b; }); + if (shotIds.size() > MAX_HISTORY_ENTRIES) { + for (size_t i = 0; i < shotIds.size() - MAX_HISTORY_ENTRIES; ++i) { + String id = shotIds[i]; + SPIFFS.remove(String("/h/") + id + ".dat"); + SPIFFS.remove(String("/h/") + id + ".json"); // remove paired notes if present + } + }
♻️ Duplicate comments (4)
src/display/plugins/ShotHistoryPlugin.cpp (4)
86-87: Use floating-point absolute value
abscan bind to the integer overload; usefabsf(orstd::fabs) for floats.- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);
20-26: Guard divide-by-zero in flow computationTwo BLE samples can share the same millisecond. Check
timeDiff > 0before dividing to avoid undefined behavior.- if (lastVolumeSample != 0) { - const unsigned long timeDiff = now - lastVolumeSample; - const float volumeDiff = weight - currentBluetoothWeight; - currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; - } + if (lastVolumeSample != 0) { + const unsigned long timeDiff = now - lastVolumeSample; + if (timeDiff > 0) { + const float volumeDiff = weight - currentBluetoothWeight; + currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; + } + }
75-84: Don’t gate extended recording onisVolumetricAvailable(); rely on recent BLE samplesPer PR objective, we should continue recording for drip even when volumetric mode isn’t “available.” Gate on recency of BLE weight instead.
- bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } - - if (!canProcessWeight) { - // If BLE connection is unstable, end extended recording early - extendedRecording = false; - return; - } + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (!hasRecentSample) { + // No recent weight updates; end extended recording + extendedRecording = false; + return; + }
147-158: Start extended recording based on recent BLE weight, not volumetric availabilityThis aligns the trigger with “3s OR until weight stabilizes” even if volumetric mode isn’t flagged as available.
- if (controller && - controller->isVolumetricAvailable() && - currentBluetoothWeight > 0) { - // Start extended recording for any shot with active weight data - extendedRecording = true; - extendedRecordingStart = millis(); - lastStableWeight = currentBluetoothWeight; - lastWeightChangeTime = 0; - return; // Don't finalize the recording yet - } + const unsigned long now = millis(); + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (hasRecentSample && currentBluetoothWeight > 0.0f) { + // Start extended recording for any shot with recent weight data + extendedRecording = true; + extendedRecordingStart = now; + lastStableWeight = currentBluetoothWeight; + lastWeightChangeTime = 0; + return; // Don't finalize the recording yet + }
🧹 Nitpick comments (6)
web/src/pages/ShotHistory/utils.js (5)
5-12: Defensive header parsing to avoid NaN timestamp and undefined profile/versionIf a corrupted/old header sneaks in,
header[2]can beundefinedandparseIntwill yieldNaN. Add a minimal length guard and fallback to keep the parser robust.- const header = lines[0].split(','); - data['version'] = header[0]; - data['profile'] = header[1]; - data['timestamp'] = parseInt(header[2], 10); + const header = lines[0].split(','); + if (header.length < 3) { + console.warn(`Invalid history header for shot ${shot.id}: "${lines[0]}"`); + return null; + } + data['version'] = header[0] || null; + data['profile'] = header[1] || null; + const ts = parseInt(header[2], 10); + data['timestamp'] = Number.isFinite(ts) ? ts : null;
24-28: Strict 11-column requirement may drop older histories; consider tolerant parsingIf backward-compatibility is desired, accept 10+ columns and default missing fields to
NaNinstead of skipping the entire line. This keeps legacy shots partially usable.- // Skip incomplete lines (should have 11 columns) - if (numbers.length !== 11) { + // Prefer 11 columns; tolerate >=10 by padding with empty values + if (numbers.length < 10) { console.warn(`Skipping incomplete line ${i}: "${lines[i]}" (${numbers.length}/11 columns)`); continue; } + while (numbers.length < 11) numbers.push('');
45-49: Validate more than just time when discarding samplesRight now only
tis validated. If every numeric payload (flows/volumes) isNaN, keeping the sample adds noise with no value. Consider dropping samples with no finite numeric data besidest.- if (isNaN(sample.t)) { + if (isNaN(sample.t)) { console.warn(`Skipping sample with invalid time: "${lines[i]}"`); continue; } + const hasAnyFinite = + [sample.v, sample.ev, sample.vf, sample.fl, sample.pf, sample.cp, sample.tf].some(Number.isFinite); + if (!hasAnyFinite) { + console.warn(`Skipping sample with no finite numeric values at line ${i}`); + continue; + }
51-60: Clamp tiny negative noise and treat sub-gram blips consistentlyBLE can jitter into tiny negatives. Clamping near-zero noise avoids regressing
maxVolume/lastValidVolume.- let currentVolume = sample.v; - if (isNaN(currentVolume) || currentVolume <= 0) { + let currentVolume = sample.v; + if (!Number.isFinite(currentVolume) || currentVolume <= 0) { currentVolume = sample.ev; } - if (!isNaN(currentVolume) && currentVolume > 0) { + // Clamp tiny negatives to 0 to avoid jitter (-0.02 etc.) + if (Number.isFinite(currentVolume) && currentVolume > 0) { lastValidVolume = currentVolume; maxVolume = Math.max(maxVolume, currentVolume); }
80-83: Gate verbose logging; avoid noisy console output in productionThe summary log is great for debugging but can flood logs. Wrap it behind a debug flag or NODE_ENV check.
- console.log(`Parsed shot ${shot.id}: duration=${data.duration}ms, volume=${data.volume}g, samples=${data['samples'].length}, maxVol=${maxVolume.toFixed(1)}`); + if (typeof process === 'undefined' || process.env.NODE_ENV !== 'production') { + console.log( + `Parsed shot ${shot.id}: duration=${data.duration}ms, volume=${data.volume}g, samples=${data['samples'].length}, maxVol=${maxVolume.toFixed(1)}` + ); + }src/display/plugins/ShotHistoryPlugin.cpp (1)
15-16: Optional: mirror BLE guard on estimation pathFor parity with BLE updates, ignore invalid
Estimationvalues (NaN/Inf) to avoid spurious fields in history.- pm->on("controller:volumetric-measurement:estimation:change", - [this](Event const &event) { currentEstimatedWeight = event.getFloat("value"); }); + pm->on("controller:volumetric-measurement:estimation:change", + [this](Event const &event) { + const float w = event.getFloat("value"); + if (!isnan(w) && isfinite(w)) currentEstimatedWeight = w; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/display/plugins/ShotHistoryPlugin.cpp(4 hunks)web/src/pages/ShotHistory/utils.js(2 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). (2)
- GitHub Check: deploy
- GitHub Check: test
🔇 Additional comments (3)
web/src/pages/ShotHistory/utils.js (1)
88-90: Nice: notes passthrough is straightforward and keeps UI wiring simpleAttaching
shot.notesdirectly todata.notesis clean and aligns with the new ShotNotes UI.src/display/plugins/ShotHistoryPlugin.cpp (2)
38-56: LGTM: unified write path withshouldRecordand header-once semanticsThe header is written once and samples are appended while
recording || extendedRecordingis true. This cleanly prevents premature closure during the post-brew window.Also applies to: 67-69
1-224: Thresholds and loop cadence verified
- EXTENDED_RECORDING_DURATION is set to 3000 ms (3 s) in ShotHistoryPlugin.h as intended.
- WEIGHT_STABILIZATION_TIME is defined as 1000 ms (1 s).
- WEIGHT_STABILIZATION_THRESHOLD is 0.1 f (0.1 g delta).
- The loop runs at a 250 ms cadence via
vTaskDelay(250 / portTICK_PERIOD_MS).All values align with the PR specification; no timing skew or mismatches detected.
dragm83
force-pushed
the
shot_record_fix
branch
from
21b4383 to
d0a4f09
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/pages/ShotHistory/utils.js (1)
31-43: Parse both 10- and 11-column samples correctly.Current code assumes 11 columns and mis-parses legacy rows. Derive v/ev based on length.
Apply this diff:
- // Parse the sample - const sample = { - t: parseInt(numbers[0], 10), - tt: parseFloat(numbers[1]), - ct: parseFloat(numbers[2]), - tp: parseFloat(numbers[3]), - cp: parseFloat(numbers[4]), - fl: parseFloat(numbers[5]), - tf: parseFloat(numbers[6]), - pf: parseFloat(numbers[7]), - vf: parseFloat(numbers[8]), - v: parseFloat(numbers[9]), - ev: parseFloat(numbers[10]), - }; + // Parse the sample (support 10- and 11-col formats) + const t = parseInt(numbers[0], 10); + const tt = parseFloat(numbers[1]); + const ct = parseFloat(numbers[2]); + const tp = parseFloat(numbers[3]); + const cp = parseFloat(numbers[4]); + const fl = parseFloat(numbers[5]); + const tf = parseFloat(numbers[6]); + const pf = parseFloat(numbers[7]); + const vf = parseFloat(numbers[8]); + // In 11-col format: [9] = v (BT weight), [10] = ev (estimated) + // In legacy 10-col: [9] = ev, no v provided + const has11 = numbers.length === 11; + const v = has11 ? parseFloat(numbers[9]) : NaN; + const ev = has11 ? parseFloat(numbers[10]) : parseFloat(numbers[9]); + + const sample = { t, tt, ct, tp, cp, fl, tf, pf, vf, v, ev };src/display/plugins/ShotHistoryPlugin.cpp (1)
3-7: Include math header for isnan/isfinite/fabsf portability.isnan/isfinite/fabsf require <math.h> (or ) across toolchains; without it, builds may fail or pick wrong overloads.
Apply this diff:
#include <display/core/utils.h> +#include <math.h> // isnan, isfinite, fabsf
♻️ Duplicate comments (4)
src/display/plugins/ShotHistoryPlugin.cpp (4)
86-86: Use fabsf for float absolute value.abs on floats can select the int overload; use fabsf (or std::fabs) to avoid UB/precision issues.
Apply this diff:
- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);
21-26: Guard divide-by-zero when computing Bluetooth flow.timeDiff can be 0 if two updates occur in the same millisecond; this risks a crash or INF.
Apply this diff:
- if (lastVolumeSample != 0) { - const unsigned long timeDiff = now - lastVolumeSample; - const float volumeDiff = weight - currentBluetoothWeight; - currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; - } + if (lastVolumeSample != 0) { + const unsigned long timeDiff = now - lastVolumeSample; + if (timeDiff > 0) { + const float volumeDiff = weight - currentBluetoothWeight; + currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; + } + }
75-84: Don’t gate extended recording on volumetric availability; rely on recent weight samples.This can prematurely stop post-brew drip capture when BLE weight is present but volumetric mode reports unavailable, defeating the PR objective.
Apply this diff:
- bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } - - if (!canProcessWeight) { - // If BLE connection is unstable, end extended recording early - extendedRecording = false; - return; - } + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (!hasRecentSample) { + // No recent weight sample; end extended recording + extendedRecording = false; + return; + }
147-159: Start extended recording based on recent weight, not volumetric availability.As above, requiring controller->isVolumetricAvailable() can skip drip capture even with active BLE data.
Apply this diff:
void ShotHistoryPlugin::endRecording() { recording = false; - if (controller && - controller->isVolumetricAvailable() && - currentBluetoothWeight > 0) { - // Start extended recording for any shot with active weight data - extendedRecording = true; - extendedRecordingStart = millis(); - lastStableWeight = currentBluetoothWeight; - lastWeightChangeTime = 0; - return; // Don't finalize the recording yet - } + const unsigned long now = millis(); + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (hasRecentSample && currentBluetoothWeight > 0.0f) { + // Start extended recording for any shot with active weight data + extendedRecording = true; + extendedRecordingStart = now; + lastStableWeight = currentBluetoothWeight; + lastWeightChangeTime = 0; + return; // Don't finalize the recording yet + } }
🧹 Nitpick comments (4)
web/src/pages/ShotHistory/utils.js (3)
8-12: Guard header parsing to avoid crashes on corrupted histories.If header has fewer than 3 fields or a non-numeric timestamp, we should bail out gracefully.
Apply this diff:
- const header = lines[0].split(','); - data['version'] = header[0]; - data['profile'] = header[1]; - data['timestamp'] = parseInt(header[2], 10); + const header = (lines[0] || '').split(','); + if (header.length < 3 || isNaN(parseInt(header[2], 10))) { + console.warn(`Invalid history header for shot ${shot.id}: "${lines[0]}"`); + return null; + } + data.version = header[0]; + data.profile = header[1]; + data.timestamp = parseInt(header[2], 10);
51-60: Use Number.isFinite for robustness and treat 0 safely.Number.isFinite avoids coercion pitfalls. If you still want to ignore zeros, keep the > 0 check; otherwise consider >= 0 to keep legitimate zeros.
Apply this diff:
- let currentVolume = sample.v; - if (isNaN(currentVolume) || currentVolume <= 0) { - currentVolume = sample.ev; - } + let currentVolume = sample.v; + if (!Number.isFinite(currentVolume) || currentVolume <= 0) { + currentVolume = sample.ev; + }
82-82: Gate verbose logs behind a debug flag.console.log every parse can be noisy in production. Prefer console.debug or guard by a DEBUG flag.
Example:
- console.log(`Parsed shot ${shot.id}: duration=${data.duration}ms, volume=${data.volume}g, samples=${data['samples'].length}, maxVol=${maxVolume.toFixed(1)}`); + if (window?.DEBUG) { + console.debug(`Parsed shot ${shot.id}: duration=${data.duration}ms, volume=${data.volume}g, samples=${data.samples.length}, maxVol=${maxVolume.toFixed(1)}`); + }src/display/plugins/ShotHistoryPlugin.cpp (1)
103-105: Optionally finalize immediately when extended recording ends.Today you defer finalization to the next loop iteration. If finalizeRecording() exists in the API, consider calling it here for determinism.
Apply this diff if finalizeRecording() is available:
if (now - extendedRecordingStart >= EXTENDED_RECORDING_DURATION) { - extendedRecording = false; + extendedRecording = false; + finalizeRecording(); }If finalizeRecording() is not present/desired, ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
schema/shot_notes.json(1 hunks)src/display/plugins/ShotHistoryPlugin.cpp(4 hunks)src/display/plugins/ShotHistoryPlugin.h(3 hunks)web/src/pages/ShotHistory/HistoryCard.jsx(2 hunks)web/src/pages/ShotHistory/ShotNotesCard.jsx(1 hunks)web/src/pages/ShotHistory/utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/pages/ShotHistory/HistoryCard.jsx
- schema/shot_notes.json
- src/display/plugins/ShotHistoryPlugin.h
- web/src/pages/ShotHistory/ShotNotesCard.jsx
⏰ 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). (2)
- GitHub Check: deploy
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web/src/pages/ShotHistory/utils.js (1)
24-28: Accept both legacy 10-col and new 11-col rows; current strict check drops valid histories.Rows produced before this PR have 10 columns (no bluetooth
v). The strict 11-col requirement rejects them and mis-parsesevasv. Parse both formats and mapv/evaccordingly.Apply this diff:
- // Skip incomplete lines (should have 11 columns) - if (numbers.length !== 11) { - console.warn(`Skipping incomplete line ${i}: "${lines[i]}" (${numbers.length}/11 columns)`); + // Skip malformed lines; support legacy 10-col and new 11-col formats + if (numbers.length < 10 || numbers.length > 11) { + console.warn(`Skipping malformed line ${i}: "${lines[i]}" (${numbers.length}/10-11 columns expected)`); continue; } - // Parse the sample - const sample = { + // Parse the sample (map v/ev depending on 10 vs 11 columns) + const vVal = numbers.length === 11 ? parseFloat(numbers[9]) : NaN; + const evVal = numbers.length === 11 ? parseFloat(numbers[10]) : parseFloat(numbers[9]); + + // Parse the sample + const sample = { t: parseInt(numbers[0], 10), tt: parseFloat(numbers[1]), ct: parseFloat(numbers[2]), tp: parseFloat(numbers[3]), cp: parseFloat(numbers[4]), fl: parseFloat(numbers[5]), tf: parseFloat(numbers[6]), pf: parseFloat(numbers[7]), vf: parseFloat(numbers[8]), - v: parseFloat(numbers[9]), - ev: parseFloat(numbers[10]), + v: vVal, + ev: evVal, };Also applies to: 31-43
🧹 Nitpick comments (5)
web/src/pages/ShotHistory/utils.js (5)
18-23: Trim and pre-validate each line to avoid parsing whitespace and trailing commas.
Whitespace-only lines and stray spaces can produce NaNs and spurious warnings.- if (!lines[i]) { - continue; - } - - const numbers = lines[i].split(','); + const raw = (lines[i] ?? '').trim(); + if (!raw) continue; + const numbers = raw.split(',').map((s) => s.trim());
14-16: Prefer a “last 1s stable weight” over raw max to align with the firmware’s stabilization rule (<0.1g in 1s).
Using the absolute max can overstate volume on noisy scales; a 1s stability window matches the PR objective.Minimal patch:
let lastValidVolume = 0; let maxVolume = 0; + // Track 1s stability window for final volume selection + let stableWindow = []; + let stableVolume = 0; @@ - if (!isNaN(currentVolume) && currentVolume > 0) { + if (!isNaN(currentVolume) && currentVolume > 0) { lastValidVolume = currentVolume; maxVolume = Math.max(maxVolume, currentVolume); + // Maintain 1s sliding window of volumes + stableWindow.push({ t: sample.t, v: currentVolume }); + while (stableWindow.length && stableWindow[0].t < sample.t - 1000) { + stableWindow.shift(); + } + const vols = stableWindow.map(e => e.v); + const spanMs = stableWindow.length + ? stableWindow[stableWindow.length - 1].t - stableWindow[0].t + : 0; + if (vols.length) { + const jitter = Math.max(...vols) - Math.min(...vols); + if (spanMs >= 1000 && jitter < 0.1) { + // Use last observed value in the stable window + stableVolume = vols[vols.length - 1]; + } + } } @@ - // Use the maximum volume seen during the shot, or fall back to last valid volume - // This handles cases where the final measurement might be slightly lower due to timing - let finalVolume = maxVolume > 0 ? maxVolume : lastValidVolume; + // Prefer the last 1s-stable weight; else maxVolume; else last valid + let finalVolume = stableVolume > 0 ? stableVolume : (maxVolume > 0 ? maxVolume : lastValidVolume);Verification: please sanity-check a few shots with known stable endpoints to ensure the reported volume matches expectations. I can add a focused unit test suite with synthetic histories covering: (a) 10-col legacy, (b) 11-col with noise/overshoot, (c) 11-col steady drip to stability.
Also applies to: 57-60, 69-81
8-12: Guard header parsing and timestamp.
If the header is malformed,timestampbecomes NaN. Fail soft with a warning.- const header = lines[0].split(','); - data['version'] = header[0]; - data['profile'] = header[1]; - data['timestamp'] = parseInt(header[2], 10); + const header = (lines[0] ?? '').split(',').map((s) => s.trim()); + data['version'] = header[0] ?? ''; + data['profile'] = header[1] ?? ''; + const ts = parseInt(header[2], 10); + if (Number.isNaN(ts)) { + console.warn(`Invalid header timestamp for shot ${shot.id}: "${header[2]}"`); + data['timestamp'] = null; + } else { + data['timestamp'] = ts; + }
82-86: Gate verbose logging in production.
Avoid noisy logs in end-user builds.- console.log(`Parsed shot ${shot.id}: duration=${data.duration}ms, volume=${data.volume}g, samples=${data['samples'].length}, maxVol=${maxVolume.toFixed(1)}`); + if (process.env.NODE_ENV !== 'production') { + console.debug(`Parsed shot ${shot.id}: duration=${data.duration}ms, volume=${data.volume}g, samples=${data['samples'].length}, maxVol=${maxVolume.toFixed(1)}`); + }
65-86: Add tests for parsing variants and final-volume selection.
Backfill tests to prevent regressions on 10/11-col formats and stabilization behavior.I can contribute a
parseHistoryData.test.jscovering:
- legacy 10-col row parsing (ev only)
- mixed rows with NaNs and blanks
- overshoot noise where stable 1s < max
- pure 11-col with valid bluetooth
v
Would you like me to open a follow-up PR with these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
web/src/pages/ShotHistory/HistoryCard.jsx(1 hunks)web/src/pages/ShotHistory/HistoryChart.jsx(1 hunks)web/src/pages/ShotHistory/index.jsx(1 hunks)web/src/pages/ShotHistory/utils.js(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- web/src/pages/ShotHistory/index.jsx
- web/src/pages/ShotHistory/HistoryChart.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/pages/ShotHistory/HistoryCard.jsx
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (1)
web/src/pages/ShotHistory/utils.js (1)
5-5: No try/catch usage: aligned with embedded error-handling constraints.
Per prior feedback, avoiding exceptions is good here; warnings are sufficient.
dragm83
force-pushed
the
shot_record_fix
branch
2 times, most recently
from
98039f8 to
b8fc1d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/display/plugins/ShotHistoryPlugin.h (1)
40-43: Wrong printf format specifier for unsigned long ‘t’ (UB risk).ShotSample::serialize() prints ‘t’ (unsigned long) with “%d”. On many toolchains this is undefined behavior and can truncate or misprint. Use “%lu”.
Apply this diff:
- return string_format("%d,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f", t, tt, ct, tp, cp, fl, tf, pf, vf, v, + return string_format("%lu,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f,%.3f", (unsigned long)t, tt, ct, tp, cp, fl, tf, pf, vf, v, ev);web/src/pages/ShotHistory/utils.js (1)
30-43: Handle v/ev indices for both 10- and 11-column formats.Current code assumes 11 columns when assigning v/ev. Add index handling to avoid reading undefined entries for legacy rows.
Apply this diff:
- // Parse the sample - const sample = { + // Parse the sample (support 10- and 11-column formats) + const has11 = numbers.length === 11; + const vVal = has11 ? parseFloat(numbers[9]) : NaN; + const evVal = has11 ? parseFloat(numbers[10]) : parseFloat(numbers[9]); + const sample = { t: parseInt(numbers[0], 10), tt: parseFloat(numbers[1]), ct: parseFloat(numbers[2]), tp: parseFloat(numbers[3]), cp: parseFloat(numbers[4]), fl: parseFloat(numbers[5]), tf: parseFloat(numbers[6]), pf: parseFloat(numbers[7]), vf: parseFloat(numbers[8]), - v: parseFloat(numbers[9]), - ev: parseFloat(numbers[10]), + v: vVal, + ev: evVal, };src/display/plugins/ShotHistoryPlugin.cpp (2)
53-55: Use correct format specifier for getTime() in header line.getTime() returns unsigned long; header writes “%ld”. Use “%lu” to avoid UB.
Apply this diff:
- file.printf("1,%s,%ld\n", currentProfileName.c_str(), getTime()); + file.printf("1,%s,%lu\n", currentProfileName.c_str(), (unsigned long)getTime());
1-7: Include math header and correct float absolute usagePlease address the following in src/display/plugins/ShotHistoryPlugin.cpp:
- Add the math header so that
isnan,isfiniteandfabsfare declared:#include <display/core/utils.h> +#include <math.h>- Replace the use of
abson floats (line 86) withfabsfto avoid integer overloads:- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);- The earlier suggestion to prefer
TaskHandle_toverxTaskHandleisn’t applicable here—there’s no task handle usage in this plugin.
♻️ Duplicate comments (5)
src/display/plugins/ShotHistoryPlugin.h (1)
61-69: Shared state updated from ISR/event and read from task without synchronization.Fields like extendedRecording, lastVolumeSample, lastWeightChangeTime, lastStableWeight, currentBluetoothWeight/Flow are written in event callbacks and read in loopTask/record(). This is a cross-core data race on ESP32.
Minimal fix:
- Mark these scalars volatile.
- Wrap multi-field reads/writes in short critical sections (portENTER_CRITICAL/EXIT) or use a mutex if acceptable.
If you want, I can propose a minimal critical-section wrapper around the weight fields and show exactly where to use it in the .cpp.
web/src/pages/ShotHistory/utils.js (1)
24-28: Accept both legacy 10-col and new 11-col history rows to preserve backward compatibility.Strictly requiring 11 columns drops older histories. Parse 10-column rows by treating the last value as ev and setting v = NaN.
Apply this diff:
- // Skip incomplete lines (should have 11 columns) - if (numbers.length !== 11) { - console.warn(`Skipping incomplete line ${i}: "${lines[i]}" (${numbers.length}/11 columns)`); + // Skip malformed lines; support legacy 10-col and new 11-col formats + if (numbers.length < 10 || numbers.length > 11) { + console.warn(`Skipping malformed line ${i}: "${lines[i]}" (${numbers.length}/10-11 columns expected)`); continue; }src/display/plugins/ShotHistoryPlugin.cpp (3)
18-29: Guard against NaN/Inf is good; add math header and divide-by-zero guard.isnan/isfinite need <math.h>/ on embedded toolchains. timeDiff can be 0 in the same-millisecond updates; guard before division.
Apply this diff:
#include <display/core/utils.h> +#include <math.h> // isnan, isfinite, fabsf @@ - if (!isnan(weight) && isfinite(weight)) { + if (!isnan(weight) && isfinite(weight)) { if (lastVolumeSample != 0) { const unsigned long timeDiff = now - lastVolumeSample; - const float volumeDiff = weight - currentBluetoothWeight; - currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; + if (timeDiff > 0) { + const float volumeDiff = weight - currentBluetoothWeight; + currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; + } }
147-164: Start extended recording based on recent BLE weight activity, not volumetric “availability.”This better matches the PR goal: keep recording for up to 3s or until weight stabilizes if weight data is present.
Apply this diff:
- if (controller && - controller->isVolumetricAvailable() && - currentBluetoothWeight > 0) { + const unsigned long now = millis(); + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (hasRecentSample && currentBluetoothWeight > 0.0f) { // Start extended recording for any shot with active weight data extendedRecording = true; extendedRecordingStart = millis(); lastStableWeight = currentBluetoothWeight; lastWeightChangeTime = 0; return; // Don't finalize the recording yet }
71-106: Extended-recording stability: use fabsf for floats and gate by recent weight samples, not volumetric availability.
- abs on floats is unsafe on Arduino/embedded and may call the int overload; use fabsf.
- Requiring controller->isVolumetricAvailable() can prematurely abort drip capture even when BLE weight data is flowing. Gate on recency of lastVolumeSample instead (e.g., <= 1s), aligning with PR intent.
Apply this diff:
- bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } - - if (!canProcessWeight) { - // If BLE connection is unstable, end extended recording early - extendedRecording = false; - return; - } + // End extended recording if we haven't seen a recent BLE weight sample + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (!hasRecentSample) { + extendedRecording = false; + return; + } @@ - const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);
🧹 Nitpick comments (5)
src/display/plugins/ShotHistoryPlugin.h (1)
10-13: Constants look good; consider scoping for clarity.Values and units are clear. Optional: move these into ShotHistoryPlugin as static constexpr to reduce global namespace clutter.
web/src/pages/ShotHistory/utils.js (1)
65-85: Volume derivation logic is sound; consider clamping tiny negatives.Using maxVolume then lastValidVolume with fallbacks is robust. Optional: clamp tiny negative values (e.g., from sensor noise) to 0 before toFixed(1).
src/display/plugins/ShotHistoryPlugin.cpp (3)
109-119: DRY: use finalizeRecording() instead of duplicating finalize logic.This block duplicates the same duration/filter/index logic in finalizeRecording(). Close the file here, then call finalizeRecording().
Apply this diff:
- unsigned long duration = millis() - shotStart; - if (duration <= 7500) { // Exclude failed shots and flushes - SPIFFS.remove("/h/" + currentId + ".dat"); - } else { - controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); - cleanupHistory(); - } + finalizeRecording();
232-237: Use SHOT_HISTORY_INTERVAL for task cadence.Keep the loop delay consistent with the configured history interval.
Apply this diff:
- vTaskDelay(250 / portTICK_PERIOD_MS); + vTaskDelay(SHOT_HISTORY_INTERVAL / portTICK_PERIOD_MS);
38-43: Consider a null-controller guard at the top of record().record() dereferences controller; a defensive early-return avoids surprises if setup order changes.
Apply this diff:
void ShotHistoryPlugin::record() { static File file; - bool shouldRecord = recording || extendedRecording; + if (!controller) return; + bool shouldRecord = recording || extendedRecording;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/display/plugins/ShotHistoryPlugin.cpp(4 hunks)src/display/plugins/ShotHistoryPlugin.h(3 hunks)web/src/pages/ShotHistory/utils.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/display/plugins/ShotHistoryPlugin.h (1)
src/display/plugins/ShotHistoryPlugin.cpp (2)
finalizeRecording(166-174)finalizeRecording(166-166)
dragm83
force-pushed
the
shot_record_fix
branch
from
b8fc1d7 to
ba349a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/pages/ShotHistory/utils.js (1)
30-43: Parse v/ev correctly for both formats (10 vs 11 cols).When accepting 10 columns, treat the last value as ev and set v = NaN; for 11 columns, parse both. This keeps downstream volume logic intact.
Apply this diff:
- // Parse the sample - const sample = { + // Parse the sample + let v = NaN, ev = NaN; + if (numbers.length === 11) { + v = parseFloat(numbers[9]); + ev = parseFloat(numbers[10]); + } else { // legacy 10-col + ev = parseFloat(numbers[9]); + } + const sample = { t: parseInt(numbers[0], 10), tt: parseFloat(numbers[1]), ct: parseFloat(numbers[2]), tp: parseFloat(numbers[3]), cp: parseFloat(numbers[4]), fl: parseFloat(numbers[5]), tf: parseFloat(numbers[6]), pf: parseFloat(numbers[7]), vf: parseFloat(numbers[8]), - v: parseFloat(numbers[9]), - ev: parseFloat(numbers[10]), + v, + ev, };src/display/plugins/ShotHistoryPlugin.cpp (1)
3-6: Include math header for isnan/isfinite/fabsf across toolchains.Some embedded toolchains won’t expose float math without <math.h>/. Add the include explicitly.
Apply this diff:
#include <display/core/utils.h> +#include <math.h> // isnan, isfinite, fabsf
♻️ Duplicate comments (5)
web/src/pages/ShotHistory/utils.js (1)
24-28: Restore backward compatibility: accept both 10- and 11-column rows.Strictly requiring 11 columns drops legacy history (10 cols) and yields empty/partial shots. Support 10 or 11 columns and only skip malformed rows. This was raised previously and appears unresolved here.
Apply this diff:
- // Skip incomplete lines (should have 11 columns) - if (numbers.length !== 11) { - console.warn(`Skipping incomplete line ${i}: "${lines[i]}" (${numbers.length}/11 columns)`); + // Skip malformed rows; support legacy 10-col and new 11-col formats + if (numbers.length < 10 || numbers.length > 11) { + console.warn(`Skipping malformed line ${i}: "${lines[i]}" (${numbers.length}/10-11 columns expected)`); continue; }src/display/plugins/ShotHistoryPlugin.cpp (4)
86-86: Use fabsf for float absolute value.abs may bind to the int overload and truncate; use fabsf to correctly compare float deltas for stabilization.
Apply this diff:
- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);
20-26: Guard against divide-by-zero when computing flow.Two BLE updates can occur within the same millisecond (timeDiff == 0). Dividing by zero yields Inf and can poison downstream logic.
Apply this diff:
if (!isnan(weight) && isfinite(weight)) { if (lastVolumeSample != 0) { const unsigned long timeDiff = now - lastVolumeSample; - const float volumeDiff = weight - currentBluetoothWeight; - currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; + if (timeDiff > 0) { + const float volumeDiff = weight - currentBluetoothWeight; + currentBluetoothFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; + } }
151-160: Start extended recording based on recent BLE samples, not volumetric availability.This aligns with “continue for 3s or until weight stabilizes” even when volumetric mode is off.
Apply this diff:
- if (controller && - controller->isVolumetricAvailable() && - currentBluetoothWeight > 0) { + const unsigned long now = millis(); + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (hasRecentSample && currentBluetoothWeight > 0.0f) { // Start extended recording for any shot with active weight data extendedRecording = true; - extendedRecordingStart = millis(); + extendedRecordingStart = now; lastStableWeight = currentBluetoothWeight; lastWeightChangeTime = 0; return; // Don't finalize the recording yet }
75-85: Extended-recording gate should rely on recent weight samples, not volumetric availability.Per PR objectives, we want to capture post-brew drip whenever BLE weight is present. Requiring controller->isVolumetricAvailable() can prematurely stop extended recording even with good BLE data.
Apply this diff:
- bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } - - if (!canProcessWeight) { + // Consider BLE weight active if we have a recent sample + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (!hasRecentSample) { // If BLE connection is unstable, end extended recording early extendedRecording = false; return; }
🧹 Nitpick comments (5)
web/src/pages/ShotHistory/utils.js (3)
51-58: Align comment with behavior or remove ev fallback.The comment says “Never take estimated volume,” yet the final fallback uses sample.ev. Either adjust the comment to reflect that ev is a last-resort fallback, or drop the ev fallback to be strict.
Apply one of:
Option A (keep ev fallback, fix comment):
- // Never take estimated volume (until fixed) - //if (isNaN(currentVolume) || currentVolume <= 0) { - // currentVolume = sample.ev; - //} + // Prefer Bluetooth weight 'v'; do not use 'ev' for running totals. + // 'ev' is only considered later as a last-resort fallback for reporting.Option B (remove ev fallback later):
- } else if (finalVolume <= 0 && !isNaN(lastSample.ev) && lastSample.ev > 0) { - finalVolume = lastSample.ev; - } + }Also applies to: 75-80
5-12: Header safety check (defensive parse).If the header is malformed, the current code will produce NaN/undefined fields. Guard for at least three header fields and bail with a warning to avoid misleading UI state.
Example change (outside current diff scope):
const header = lines[0].split(','); if (header.length < 3) { console.warn(`Invalid history header for shot ${shot.id}: "${lines[0]}"`); return null; }
84-85: Tone down verbose logging in production.Consider console.debug or gate logs behind a dev flag; this line runs for every parsed shot and can get noisy.
Apply this diff:
- console.log(`Parsed shot ${shot.id}: duration=${data.duration}ms, volume=${data.volume}g, samples=${data['samples'].length}, maxVol=${maxVolume.toFixed(1)}`); + console.debug(`Parsed shot ${shot.id}: duration=${data.duration}ms, volume=${data.volume}g, samples=${data['samples'].length}, maxVol=${maxVolume.toFixed(1)}`);src/display/plugins/ShotHistoryPlugin.cpp (2)
109-119: De-duplicate finalization logic by calling finalizeRecording().record() repeats the same post-shot logic that exists in finalizeRecording(). Close the file, then delegate to finalizeRecording() to avoid drift.
Apply this diff:
- if (!recording && !extendedRecording && isFileOpen) { - file.close(); - isFileOpen = false; - unsigned long duration = millis() - shotStart; - if (duration <= 7500) { // Exclude failed shots and flushes - SPIFFS.remove("/h/" + currentId + ".dat"); - } else { - controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); - cleanupHistory(); - } - } + if (!recording && !extendedRecording && isFileOpen) { + file.close(); + isFileOpen = false; + finalizeRecording(); + }
232-238: Minor concurrency note (optional).Event callbacks and loopTask run on different contexts; shared fields (currentBluetoothWeight, lastVolumeSample, currentBluetoothFlow) may benefit from being marked volatile or copied to locals in record() before multi-step logic. ESP32 32-bit float writes are typically atomic, so treat this as optional hardening.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/display/plugins/ShotHistoryPlugin.cpp(4 hunks)src/display/plugins/ShotHistoryPlugin.h(3 hunks)web/src/pages/ShotHistory/utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/display/plugins/ShotHistoryPlugin.h
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (2)
src/display/plugins/ShotHistoryPlugin.cpp (2)
14-18: Good removal of try/catch in embedded paths.Event processing now uses guards and simple validity checks—safer and leaner on embedded toolchains.
Also applies to: 20-33
103-105: Constants Verified – All stabilization constants match the intended UX thresholds.
EXTENDED_RECORDING_DURATIONis defined as3000(ms) (3 s).WEIGHT_STABILIZATION_TIMEis defined as1000(ms) (1 s).WEIGHT_STABILIZATION_THRESHOLDis defined as0.1f(0.1 g).No changes required.
dragm83
force-pushed
the
shot_record_fix
branch
2 times, most recently
from
6012893 to
fb0cb36
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
web/src/pages/ShotHistory/utils.js (1)
24-28: Enforce backward compatibility for 10 and 11 column formats.The strict 11-column requirement will break compatibility with older shot history files that likely have 10 columns. The system should gracefully handle both formats.
Apply this diff to accept both formats:
- // Skip incomplete lines (should have 11 columns) - if (numbers.length !== 11) { - console.warn(`Skipping incomplete line ${i}: "${lines[i]}" (${numbers.length}/11 columns)`); + // Skip malformed lines; support legacy 10-col and new 11-col formats + if (numbers.length < 10 || numbers.length > 11) { + console.warn(`Skipping malformed line ${i}: "${lines[i]}" (${numbers.length} columns, expected 10-11)`); continue; }Then handle both formats when parsing:
// After the validation check if (numbers.length === 10) { // Legacy format: last value is ev, no v sample.v = NaN; sample.ev = parseFloat(numbers[9]); sample.pr = NaN; } else { // New format with all 11 columns sample.v = parseFloat(numbers[9]); sample.ev = parseFloat(numbers[10]); sample.pr = parseFloat(numbers[11]); }src/display/plugins/ShotHistoryPlugin.h (2)
27-28: ArduinoJson API compatibility issue with JsonDocument parameters.Using
JsonDocumentdirectly in function signatures causes issues with ArduinoJson v6. The API should useJsonVarianttypes instead.Apply this diff:
- void saveNotes(const String &id, const JsonDocument ¬es); - void loadNotes(const String &id, JsonDocument ¬es); + void saveNotes(const String &id, JsonVariantConst notes); + bool loadNotes(const String &id, JsonVariant dst);
64-72: Thread safety concern with shared state variables.Multiple member variables are accessed across different contexts (ISR/event callbacks and FreeRTOS tasks) without synchronization, which could lead to data races on the dual-core ESP32.
At minimum, mark the shared scalar fields as
volatile:- bool recording = false; - bool headerWritten = false; - bool extendedRecording = false; - unsigned long shotStart = 0; - unsigned long extendedRecordingStart = 0; - unsigned long lastVolumeSample = 0; - unsigned long lastWeightChangeTime = 0; - float currentTemperature = 0.0f; - float currentBluetoothWeight = 0.0f; - float lastStableWeight = 0.0f; - float currentBluetoothFlow = 0.0f; + volatile bool recording = false; + volatile bool headerWritten = false; + volatile bool extendedRecording = false; + volatile unsigned long shotStart = 0; + volatile unsigned long extendedRecordingStart = 0; + volatile unsigned long lastVolumeSample = 0; + volatile unsigned long lastWeightChangeTime = 0; + volatile float currentTemperature = 0.0f; + volatile float currentBluetoothWeight = 0.0f; + volatile float lastStableWeight = 0.0f; + volatile float currentBluetoothFlow = 0.0f;For full thread safety, consider using FreeRTOS mutexes or critical sections to protect multi-field updates.
src/display/plugins/ShotHistoryPlugin.cpp (7)
18-28: Replace exception handling with explicit guards.Following the PR reviewer's request (spsancti), exception handling should be avoided in embedded systems as it may not function reliably.
Remove the implicit exception handling and add explicit validation:
pm->on("controller:volumetric-measurement:bluetooth:change", [this](Event const &event) { const float weight = event.getFloat("value"); + // Validate weight value + if (!isfinite(weight)) return; + const unsigned long now = millis(); if (lastVolumeSample != 0) { const unsigned long timeDiff = now - lastVolumeSample; + // Guard against divide by zero + if (timeDiff == 0) return; const float volumeDiff = weight - currentBluetoothWeight; const float volumeFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; currentBluetoothFlow = currentBluetoothFlow * 0.9f + volumeFlow * 0.1f; } lastVolumeSample = now; currentBluetoothWeight = weight; });Also add
#include <cmath>at the top forisfinite.
212-216: Fix ArduinoJson memory management issues.Creating separate
JsonDocumentinstances and copying them causes allocation issues with ArduinoJson v6.For the list endpoint:
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - o["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, o["notes"])) { + o.remove("notes"); + }For the get endpoint:
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - response["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, response["notes"])) { + response.remove("notes"); + }Also applies to: 230-234
244-252: Avoid JsonDocument construction in request handlers.Similar ArduinoJson v6 issues with creating intermediate
JsonDocumentobjects.} else if (type == "req:history:notes:get") { - auto id = request["id"].as<String>(); - JsonDocument notes; - loadNotes(id, notes); - response["notes"] = notes; + String id = request["id"].as<String>(); + if (!loadNotes(id, response["notes"])) { + response["notes"] = JsonObject(); // empty object if not found + } } else if (type == "req:history:notes:save") { const String id = request["id"].as<String>(); - const JsonDocument& notesDoc = request["notes"]; - - saveNotes(id, notesDoc); + JsonVariantConst notes = request["notes"]; + if (notes.isNull()) { + response["error"] = "invalid notes"; + return; + } + // Add timestamp to notes + JsonObject notesObj = request["notes"].as<JsonObject>(); + notesObj["timestamp"] = getTime(); + saveNotes(id, notes); + response["msg"] = "Ok";
257-274: Refactor saveNotes/loadNotes to use JsonVariant.This avoids intermediate allocations and aligns with ArduinoJson v6 best practices.
-void ShotHistoryPlugin::saveNotes(const String &id, const JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); - if (file) { - String notesStr; - serializeJson(notes, notesStr); - file.print(notesStr); - file.close(); - } -} +void ShotHistoryPlugin::saveNotes(const String &id, JsonVariantConst notes) { + File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); + if (!file) return; + serializeJson(notes, file); + file.close(); +} -void ShotHistoryPlugin::loadNotes(const String &id, JsonDocument ¬es) { - File file = SPIFFS.open("/h/" + id + ".json", "r"); - if (file) { - String notesStr = file.readString(); - file.close(); - deserializeJson(notes, notesStr); - } -} +bool ShotHistoryPlugin::loadNotes(const String &id, JsonVariant dst) { + File file = SPIFFS.open("/h/" + id + ".json", "r"); + if (!file) return false; + DeserializationError err = deserializeJson(dst, file); + file.close(); + return !err; +}
72-81: Extended recording gating is too restrictive.The current implementation requires
controller->isVolumetricAvailable()which may prevent extended recording even when Bluetooth weight data is actually available. This contradicts the PR's objective to capture post-brew drip whenever weight data exists.Replace the volumetric availability check with a check for recent weight samples:
- bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } - - if (!canProcessWeight) { + // Consider weight active if we've seen a recent sample + const bool hasRecentSample = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (!hasRecentSample) { // If BLE connection is unstable, end extended recording early extendedRecording = false; + finalizeRecording(); return; }
83-83: Use the correct floating-point absolute value function.The
absfunction may resolve to the integer overload, causing incorrect behavior with float values.- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);Don't forget to include
<cmath>at the top of the file.
80-80: Missing call to finalizeRecording().When ending extended recording due to unstable BLE connection, the code should call
finalizeRecording()to properly close the recording session.if (!canProcessWeight) { // If BLE connection is unstable, end extended recording early extendedRecording = false; + finalizeRecording(); return; }
🧹 Nitpick comments (3)
web/src/pages/ShotHistory/utils.js (2)
52-58: Consider re-enabling estimated volume as fallback.The code has commented out the fallback to estimated volume. While I understand there might be issues with the estimated values, having no fallback at all could result in missing volume data for shots where Bluetooth weight is unavailable.
Consider adding a flag or configuration option to control whether to use estimated volume:
// Add at the top of the function const USE_ESTIMATED_VOLUME = false; // Can be made configurable // Then in the tracking logic if (USE_ESTIMATED_VOLUME && (isNaN(currentVolume) || currentVolume <= 0)) { currentVolume = sample.ev; }
71-71: Missing indentation on line 71.The variable declaration lacks proper indentation.
-let finalVolume = maxVolume > 0 ? maxVolume : lastValidVolume; + let finalVolume = maxVolume > 0 ? maxVolume : lastValidVolume;src/display/plugins/ShotHistoryPlugin.cpp (1)
149-158: Extended recording logic looks good but could be more inclusive.The implementation correctly starts extended recording when weight data is available. However, similar to the earlier comment, requiring
isVolumetricAvailable()might be too restrictive.Consider checking for recent weight updates instead:
- if (controller && - controller->isVolumetricAvailable() && - currentBluetoothWeight > 0) { + // Start extended recording if we have recent weight data + const unsigned long now = millis(); + const bool hasRecentWeight = (lastVolumeSample != 0) && ((now - lastVolumeSample) <= 1000); + if (hasRecentWeight && currentBluetoothWeight > 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/display/plugins/ShotHistoryPlugin.cpp(4 hunks)src/display/plugins/ShotHistoryPlugin.h(3 hunks)web/src/pages/ShotHistory/utils.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/display/plugins/ShotHistoryPlugin.h (1)
src/display/plugins/ShotHistoryPlugin.cpp (2)
finalizeRecording(164-172)finalizeRecording(164-164)
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (2)
src/display/plugins/ShotHistoryPlugin.h (1)
10-13: Well-designed constants for extended recording behavior.The constants for extended recording duration, stabilization time, and threshold are appropriately chosen and align well with the PR objectives.
src/display/plugins/ShotHistoryPlugin.cpp (1)
164-172: Good implementation of shot cleanup logic.The
finalizeRecording()method correctly handles cleanup of short/failed shots (< 7.5 seconds) and properly advances the history index for valid shots.
| } | ||
| // Weight has been stable for the threshold time, stop extended recording | ||
| if (now - lastWeightChangeTime >= WEIGHT_STABILIZATION_TIME) { | ||
| extendedRecording = false; |
There was a problem hiding this comment.
Missing finalizeRecording() calls when extended recording ends.
When extended recording ends due to weight stabilization or timeout, finalizeRecording() should be called to properly complete the recording process.
// Weight has been stable for the threshold time, stop extended recording
if (now - lastWeightChangeTime >= WEIGHT_STABILIZATION_TIME) {
extendedRecording = false;
+ finalizeRecording();
} // Also stop extended recording after maximum duration
if (now - extendedRecordingStart >= EXTENDED_RECORDING_DURATION) {
extendedRecording = false;
+ finalizeRecording();
}Also applies to: 101-101
🤖 Prompt for AI Agents
In src/display/plugins/ShotHistoryPlugin.cpp around lines 91 and 101, when
extendedRecording is set to false due to weight stabilization or timeout you
must call finalizeRecording() to properly finish the recording; update both
branches where extendedRecording = false to invoke finalizeRecording()
immediately before or after flipping the flag, and ensure any necessary state
guards (e.g., only call if a recording is active) are checked to avoid
double-finalization.
fb0cb36 to
7221ac2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/pages/ShotHistory/utils.js (1)
31-44: Index-based parsing will mis-map legacy 10/11-col rows; parse defensively with length-aware indices.When length is 10,
evis typically at index 9 andv/prare absent. For 11,vis at 9 andevat 10, with nopr. For 12,pris at 11. Parse accordingly to avoid incorrect field assignments.Apply this diff:
- const sample = { - t: parseInt(numbers[0], 10), - tt: parseFloat(numbers[1]), - ct: parseFloat(numbers[2]), - tp: parseFloat(numbers[3]), - cp: parseFloat(numbers[4]), - fl: parseFloat(numbers[5]), - tf: parseFloat(numbers[6]), - pf: parseFloat(numbers[7]), - vf: parseFloat(numbers[8]), - v: parseFloat(numbers[9]), - ev: parseFloat(numbers[10]), - pr: parseFloat(numbers[11]), - }; + const L = numbers.length; + const num = (idx) => (idx >= 0 && idx < L ? parseFloat(numbers[idx]) : NaN); + // 10 cols => ev at 9 (no v/pr) + // 11 cols => v at 9, ev at 10 (no pr) + // 12 cols => v at 9, ev at 10, pr at 11 + const evIdx = L === 10 ? 9 : 10; + const sample = { + t: parseInt(numbers[0], 10), + tt: num(1), + ct: num(2), + tp: num(3), + cp: num(4), + fl: num(5), + tf: num(6), + pf: num(7), + vf: num(8), + v: L >= 11 ? num(9) : NaN, + ev: num(evIdx), + pr: L >= 12 ? num(11) : NaN, + };
♻️ Duplicate comments (1)
web/src/pages/ShotHistory/utils.js (1)
24-28: Strict 12-column requirement reintroduces backward-compatibility break; accept 10–12 columns.Older history rows are 10 or 11 columns. Dropping them yields empty/partial histories. Accept 10, 11, and 12 columns, and only skip rows outside this range. This also aligns with the PR goal of improving history robustness.
Apply this diff:
- // Skip incomplete lines (should have 12 columns: t,tt,ct,tp,cp,fl,tf,pf,vf,v,ev,pr) - if (numbers.length !== 12) { - console.warn(`Skipping incomplete line ${i}: "${lines[i]}" (${numbers.length}/12 columns)`); + // Accept legacy 10/11 and new 12-column rows; skip only malformed rows + if (numbers.length < 10 || numbers.length > 12) { + console.warn(`Skipping malformed line ${i}: "${lines[i]}" (${numbers.length}/10–12 cols expected)`); continue; }
🧹 Nitpick comments (2)
web/src/pages/ShotHistory/utils.js (2)
46-50: Validate monotonic, non-negative time to drop out-of-order or corrupt samples.This guards duration computation and downstream charts.
Apply this diff:
- if (isNaN(sample.t)) { + if (isNaN(sample.t) || sample.t < 0 || sample.t < prevTime) { console.warn(`Skipping sample with invalid time: "${lines[i]}"`); continue; } + prevTime = sample.t;
26-27: Reduce console noise; throttle or downgrade per-line warnings.Per-sample warnings can flood the console for long shots. Consider batching counts or downgrading to
console.debugbehind a verbose flag.Also applies to: 48-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
web/src/pages/ShotHistory/utils.js(2 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). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (1)
web/src/pages/ShotHistory/utils.js (1)
68-81: Review suggestion references undefined variables – please ignore or correctThe proposed diff assumes the existence of
maxEv,lastValidEv, andanyValidV, but none of these are declared or tracked inweb/src/pages/ShotHistory/utils.js. Applying this change as-is will introduce runtime errors. If you intend to fall back on estimated volumes (ev), you must first:
- Introduce and initialize
let maxEv = 0andlet lastValidEv = 0alongside the existing volume trackers.- Within the sample loop, update these trackers when
sample.evis a valid positive number.- Declare and maintain a flag like
anyValidVif you need to distinguish “no real volume seen” cases.Until those foundations are in place, the suggested patch is not applicable.
Likely an incorrect or invalid review comment.
| let lastValidVolume = 0; | ||
| let maxVolume = 0; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Track estimated volume maxima and add state for time validation.
If a shot has no reliable v column (legacy 10-col), current logic never computes a max volume across ev and may under-report. Track maxEv/lastValidEv, a flag for whether any real v was seen, and keep prevTime to validate monotonic time.
Apply this diff:
let lastValidVolume = 0;
let maxVolume = 0;
+ // Track estimated volume for legacy rows and absence of real 'v'
+ let lastValidEv = 0;
+ let maxEv = 0;
+ let anyValidV = false;
+ // Ensure time is monotonic
+ let prevTime = -Infinity;📝 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.
| let lastValidVolume = 0; | |
| let maxVolume = 0; | |
| let lastValidVolume = 0; | |
| let maxVolume = 0; | |
| // Track estimated volume for legacy rows and absence of real 'v' | |
| let lastValidEv = 0; | |
| let maxEv = 0; | |
| let anyValidV = false; | |
| // Ensure time is monotonic | |
| let prevTime = -Infinity; |
🤖 Prompt for AI Agents
In web/src/pages/ShotHistory/utils.js around lines 14 to 16, replace the two
simple volume counters with explicit state for estimated volumes and time
validation: add variables maxEv = 0 and lastValidEv = 0 to track maxima across
ev when v is missing, add anyRealVSeen = false to mark when a real v column has
been observed, and add prevTime = -Infinity to validate monotonic time; then
update parsing logic so that for each row you use v when present (set
anyRealVSeen = true and update lastValidEv/maxEv) otherwise consider ev for
maxEv/lastValidEv, and before accepting a row check that its time is > prevTime
(update prevTime only for accepted rows) to skip non-monotonic timestamps.
| // Track valid volume values (prefer bluetooth weight 'v', fall back to estimated 'ev') | ||
| let currentVolume = sample.v; | ||
|
|
||
| // Never take estimated volume (until fixed) | ||
| //if (isNaN(currentVolume) || currentVolume <= 0) { | ||
| // currentVolume = sample.ev; | ||
| //} | ||
|
|
||
| if (!isNaN(currentVolume) && currentVolume > 0) { | ||
| lastValidVolume = currentVolume; | ||
| maxVolume = Math.max(maxVolume, currentVolume); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
If no 'v' exists, compute max from 'ev' so volume isn’t lost on legacy rows.
Right now, maxVolume is never updated from ev. For legacy data this yields null/low volumes even though ev is usable.
Apply this diff:
// Track valid volume values (prefer bluetooth weight 'v', fall back to estimated 'ev')
let currentVolume = sample.v;
@@
- if (!isNaN(currentVolume) && currentVolume > 0) {
- lastValidVolume = currentVolume;
- maxVolume = Math.max(maxVolume, currentVolume);
- }
+ if (!isNaN(currentVolume) && currentVolume > 0) {
+ anyValidV = true;
+ lastValidVolume = currentVolume;
+ maxVolume = Math.max(maxVolume, currentVolume);
+ } else if (!isNaN(sample.ev) && sample.ev > 0) {
+ // Only track ev when 'v' is absent to avoid mixing signals
+ lastValidEv = sample.ev;
+ maxEv = Math.max(maxEv, sample.ev);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In web/src/pages/ShotHistory/utils.js around lines 52 to 63, the code never
updates maxVolume from the legacy estimated field ev when v is missing; change
the logic so that if sample.v is missing/invalid (isNaN or <= 0) you fall back
to sample.ev (if numeric and > 0) for computing maxVolume and updating
lastValidVolume — keep preferring v when present but use ev as a fallback to
avoid losing legacy volume data.
2c6c704 to
1fbc139
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/display/plugins/ShotHistoryPlugin.cpp (3)
18-28: Add divide-by-zero guard for flow calculationWhen two BLE updates arrive in the same millisecond,
timeDiffcould be 0, causing undefined behavior in the flow calculation.pm->on("controller:volumetric-measurement:bluetooth:change", [this](Event const &event) { const float weight = event.getFloat("value"); const unsigned long now = millis(); if (lastVolumeSample != 0) { const unsigned long timeDiff = now - lastVolumeSample; + if (timeDiff > 0) { const float volumeDiff = weight - currentBluetoothWeight; const float volumeFlow = volumeDiff / static_cast<float>(timeDiff) * 1000.0f; currentBluetoothFlow = currentBluetoothFlow * 0.9f + volumeFlow * 0.1f; + } } lastVolumeSample = now; currentBluetoothWeight = weight; });
212-216: Potential ArduinoJson memory issues with JsonDocument copyingCreating separate
JsonDocumentinstances and copying them can cause memory allocation issues with ArduinoJson v6. Consider deserializing directly into the response document.For the list endpoint:
// Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - o["notes"] = notes; - } + if (!loadNotes(id, o["notes"])) { + o.remove("notes"); + }For the get endpoint:
// Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - response["notes"] = notes; - } + if (!loadNotes(id, response["notes"])) { + response.remove("notes"); + }Update the
loadNotessignature to return bool and work with JsonVariant:-void ShotHistoryPlugin::loadNotes(const String &id, JsonDocument ¬es) { +bool ShotHistoryPlugin::loadNotes(const String &id, JsonVariant notes) { File file = SPIFFS.open("/h/" + id + ".json", "r"); - if (file) { - String notesStr = file.readString(); - file.close(); - deserializeJson(notes, notesStr); - } + if (!file) return false; + String notesStr = file.readString(); + file.close(); + DeserializationError err = deserializeJson(notes, notesStr); + return !err && notes.size() > 0; }Also applies to: 230-234
250-253: Missing validation and timestamp for notes saveThe notes save handler doesn't validate the incoming notes or add a timestamp as mentioned in the past review comments.
} else if (type == "req:history:notes:save") { const String id = request["id"].as<String>(); - const JsonDocument& notesDoc = request["notes"]; + JsonObject notes = request["notes"].as<JsonObject>(); + if (notes.isNull()) { + response["error"] = "invalid notes"; + return; + } + notes["timestamp"] = getTime(); + saveNotes(id, notes); + response["msg"] = "Ok"; - saveNotes(id, notesDoc); - }
♻️ Duplicate comments (3)
web/src/pages/ShotHistory/utils.js (1)
24-28: Consider backward compatibility with 10-column formatThe strict check for exactly 12 columns may break compatibility with older history data that uses 10 or 11 columns. Consider accepting both formats to maintain backward compatibility.
src/display/plugins/ShotHistoryPlugin.cpp (2)
83-83: Use fabsf for floating-point absolute valueUsing
abson floats is implementation-defined and may resolve to the integer overload. Usefabsforstd::fabsfor correct floating-point absolute value calculation.+#include <cmath> // Add at the top with other includes - const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight);
72-75: Extended recording should rely on recent weight samples, not volumetric availabilityThe current check for
controller->isVolumetricAvailable()may prevent extended recording even when BLE weight data is present. This doesn't align with the PR's goal to capture post-brew drip whenever weight data is available.- bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } + // Consider BLE weight active if we've seen a recent sample + const bool hasRecentSample = (lastVolumeSample != 0) && + ((now - lastVolumeSample) <= 1000); - if (!canProcessWeight) { + if (!hasRecentSample) { - // If BLE connection is unstable, end extended recording early + // No recent weight sample, end extended recording early extendedRecording = false; + finalizeRecording(); return; }
🧹 Nitpick comments (2)
web/src/pages/ShotHistory/utils.js (1)
52-63: Fallback to estimated volume is disabled but might be needed for legacy dataThe code comments out the fallback to
sample.evwhensample.vis invalid. For shots without Bluetooth weight data (legacy or when BLE is unavailable), this could result in missing volume data entirely. Consider keeping the fallback active for backward compatibility.Enable the fallback to estimated volume:
- // Never take estimated volume (until fixed) - //if (isNaN(currentVolume) || currentVolume <= 0) { - // currentVolume = sample.ev; - //} + // Fall back to estimated volume when Bluetooth weight is unavailable + if (isNaN(currentVolume) || currentVolume <= 0) { + currentVolume = sample.ev; + }src/display/plugins/ShotHistoryPlugin.cpp (1)
164-172: Duplicate cleanup logic between record() and finalizeRecording()The
finalizeRecording()method duplicates the cleanup logic that's already in therecord()method (lines 110-116). This could lead to maintenance issues if the logic needs to change.Consider consolidating the cleanup logic to avoid duplication:
void ShotHistoryPlugin::finalizeRecording() { unsigned long duration = millis() - shotStart; if (duration <= 7500) { // Exclude failed shots and flushes SPIFFS.remove("/h/" + currentId + ".dat"); + SPIFFS.remove("/h/" + currentId + ".json"); // Also remove notes file } else { controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); cleanupHistory(); } }Then update the record() method to call finalizeRecording():
- if (!recording && !extendedRecording && isFileOpen) { + if (!recording && !extendedRecording) { + if (isFileOpen) { file.close(); isFileOpen = false; - unsigned long duration = millis() - shotStart; - if (duration <= 7500) { // Exclude failed shots and flushes - SPIFFS.remove("/h/" + currentId + ".dat"); - SPIFFS.remove("/h/" + currentId + ".json"); // Also remove notes file if it exists - } else { - controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); - cleanupHistory(); - } + } + finalizeRecording(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/display/plugins/ShotHistoryPlugin.cpp(4 hunks)src/display/plugins/ShotHistoryPlugin.h(3 hunks)web/src/pages/ShotHistory/utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/display/plugins/ShotHistoryPlugin.h
⏰ 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). (2)
- GitHub Check: deploy
- GitHub Check: test
🔇 Additional comments (2)
web/src/pages/ShotHistory/utils.js (1)
71-80: Good implementation of multiple volume fallbacksThe multi-level fallback strategy (maxVolume → lastValidVolume → lastSample.v → lastSample.ev) provides robust volume determination, especially for shots with post-brew drip.
src/display/plugins/ShotHistoryPlugin.cpp (1)
145-162: Well-structured transition to extended recordingThe implementation correctly checks for available weight data before starting extended recording and properly initializes all necessary state variables. The fallback to immediate finalization for shots without weight data is appropriate.
| extendedRecording = false; | ||
| return; |
There was a problem hiding this comment.
Missing finalizeRecording() calls when extended recording ends
When extended recording ends (due to BLE unavailability, weight stabilization, or timeout), finalizeRecording() should be called to properly complete the recording process and perform cleanup.
if (!canProcessWeight) {
// If BLE connection is unstable, end extended recording early
extendedRecording = false;
+ finalizeRecording();
return;
}
// ... weight stabilization check ...
// Weight has been stable for the threshold time, stop extended recording
if (now - lastWeightChangeTime >= WEIGHT_STABILIZATION_TIME) {
extendedRecording = false;
+ finalizeRecording();
}
// ...
// Also stop extended recording after maximum duration
if (now - extendedRecordingStart >= EXTENDED_RECORDING_DURATION) {
extendedRecording = false;
+ finalizeRecording();
}Also applies to: 91-91, 101-101
🤖 Prompt for AI Agents
In src/display/plugins/ShotHistoryPlugin.cpp around lines 79-80 (and similarly
at lines 91 and 101), the code sets extendedRecording = false and returns
without calling finalizeRecording(); modify these exit paths so that before
setting extendedRecording to false and returning you call finalizeRecording()
(handling any errors/guards as appropriate), ensuring recording cleanup always
runs when extended recording ends due to BLE unavailability, weight
stabilization, or timeout.
76cdf10 to
54753d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/display/plugins/ShotHistoryPlugin.cpp (2)
3-7: Include for fabsf/isfinite usage.Required for correct float math on embedded toolchains.
Apply:
#include <display/core/utils.h> +#include <cmath>
100-111: Avoid double-increment and centralize finalization; call finalizeRecording() here.Currently, finalizeRecording() also increments history index, and this block duplicates that, causing a double-increment when finalizeRecording() is called from endRecording(). Centralize in one place.
Apply:
- if (!recording && !extendedRecording && isFileOpen) { - file.close(); - isFileOpen = false; - unsigned long duration = millis() - shotStart; - if (duration <= 7500) { // Exclude failed shots and flushes - SPIFFS.remove("/h/" + currentId + ".dat"); - SPIFFS.remove("/h/" + currentId + ".json"); // Also remove notes file if it exists - } else { - controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); - cleanupHistory(); - } - } + if (!recording && !extendedRecording && isFileOpen) { + file.close(); + isFileOpen = false; + finalizeRecording(); + }And update finalizeRecording() to also remove the notes file for short shots (see below).
♻️ Duplicate comments (9)
web/src/pages/ShotHistory/utils.js (3)
24-28: Don’t hard-require 12 columns; accept legacy 10–12 to avoid dropping history.Strict 12-col check breaks older records. Allow 10–12 and warn only on malformed rows.
Apply:
- // Skip incomplete lines (should have 12 columns: t,tt,ct,tp,cp,fl,tf,pf,vf,v,ev,pr) - if (numbers.length !== 12) { - console.warn(`Skipping incomplete line ${i}: "${lines[i]}" (${numbers.length}/12 columns)`); - continue; - } + // Accept legacy 10–12 columns; skip malformed rows + if (numbers.length < 10 || numbers.length > 12) { + console.warn(`Skipping malformed line ${i}: "${lines[i]}" (${numbers.length}/10-12 columns expected)`); + continue; + }
14-16: Track ev as a fallback path for legacy rows (max and last).Without tracking ev, legacy entries under-report volume.
Apply:
let lastValidVolume = 0; let maxVolume = 0; + let lastValidEv = 0; + let maxEv = 0; + let anyValidV = false;
52-63: Prefer v but fall back to ev when v is missing; update maxima accordingly.Ensures correct volume derivation for legacy data.
Apply:
- // Track valid volume values (prefer bluetooth weight 'v', fall back to estimated 'ev') - let currentVolume = sample.v; - - // Never take estimated volume (until fixed) - //if (isNaN(currentVolume) || currentVolume <= 0) { - // currentVolume = sample.ev; - //} - - if (!isNaN(currentVolume) && currentVolume > 0) { - lastValidVolume = currentVolume; - maxVolume = Math.max(maxVolume, currentVolume); - } + // Track valid volume values (prefer 'v'; fall back to 'ev' for legacy rows) + const currentVolume = sample.v; + if (!isNaN(currentVolume) && currentVolume > 0) { + anyValidV = true; + lastValidVolume = currentVolume; + maxVolume = Math.max(maxVolume, currentVolume); + } else if (!isNaN(sample.ev) && sample.ev > 0) { + lastValidEv = sample.ev; + maxEv = Math.max(maxEv, sample.ev); + }src/display/plugins/ShotHistoryPlugin.cpp (6)
204-210: Deserialize notes directly into response; avoid intermediate JsonDocument.Prevents ArduinoJson heap churn and copy.
Apply:
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - o["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, o["notes"])) { + o.remove("notes"); + }
223-227: Same: load notes into response document in-place.Apply:
- // Also include notes if they exist - JsonDocument notes; - loadNotes(id, notes); - if (!notes.isNull() && notes.size() > 0) { - response["notes"] = notes; - } + // Also include notes if they exist + if (!loadNotes(id, response["notes"])) { + response.remove("notes"); + }
237-245: Notes handlers: avoid constructing JsonDocument; validate input and set timestamp safely.Apply:
- } else if (type == "req:history:notes:get") { - auto id = request["id"].as<String>(); - JsonDocument notes; - loadNotes(id, notes); - response["notes"] = notes; - } else if (type == "req:history:notes:save") { - const String id = request["id"].as<String>(); - const JsonDocument ¬esDoc = request["notes"]; - - saveNotes(id, notesDoc); + } else if (type == "req:history:notes:get") { + auto id = request["id"].as<String>(); + if (!loadNotes(id, response["notes"])) { + response["notes"] = JsonObject(); // empty object if not found + } + } else if (type == "req:history:notes:save") { + const String id = request["id"].as<String>(); + JsonObject notes = request["notes"].as<JsonObject>(); + if (notes.isNull()) { + response["error"] = "invalid notes"; + return; + } + notes["timestamp"] = getTime(); + saveNotes(id, notes); response["msg"] = "Ok";Note: requires updated saveNotes/loadNotes signatures below.
249-266: Refactor saveNotes/loadNotes to use JsonVariant and stream directly.Reduces allocations and copies.
Apply:
-void ShotHistoryPlugin::saveNotes(const String &id, const JsonDocument ¬es) { +void ShotHistoryPlugin::saveNotes(const String &id, JsonVariantConst notes) { File file = SPIFFS.open("/h/" + id + ".json", FILE_WRITE); - if (file) { - String notesStr; - serializeJson(notes, notesStr); - file.print(notesStr); - file.close(); - } + if (!file) return; + serializeJson(notes, file); + file.close(); } -void ShotHistoryPlugin::loadNotes(const String &id, JsonDocument ¬es) { +bool ShotHistoryPlugin::loadNotes(const String &id, JsonVariant dst) { File file = SPIFFS.open("/h/" + id + ".json", "r"); - if (file) { - String notesStr = file.readString(); - file.close(); - deserializeJson(notes, notesStr); - } + if (!file) return false; + String notesStr = file.readString(); + file.close(); + return deserializeJson(dst, notesStr) == DeserializationError::Ok; }And update declarations in ShotHistoryPlugin.h:
// in class ShotHistoryPlugin void saveNotes(const String &id, JsonVariantConst notes); bool loadNotes(const String &id, JsonVariant dst);
77-96: Use fabsf for floats and finalize immediately when ending extended recording.abs() may pick int overload; also ensure we finalize on both stabilization and timeout.
Apply:
- const float weightDiff = abs(currentBluetoothWeight - lastStableWeight); + const float weightDiff = fabsf(currentBluetoothWeight - lastStableWeight); @@ - if (now - lastWeightChangeTime >= WEIGHT_STABILIZATION_TIME) { - extendedRecording = false; - } + if (now - lastWeightChangeTime >= WEIGHT_STABILIZATION_TIME) { + extendedRecording = false; + // Defer file close to the finalization path + } @@ - if (now - extendedRecordingStart >= EXTENDED_RECORDING_DURATION) { - extendedRecording = false; - } + if (now - extendedRecordingStart >= EXTENDED_RECORDING_DURATION) { + extendedRecording = false; + }Follow-up (see next comment) centralizes finalization to prevent double-increment.
66-75: Don’t gate extended recording on volumetric availability; and avoid early return without finalization.This can suppress post-brew drip capture and leaves finalization timing to a later tick.
Apply either:
- Minimal: remove this block and rely on stabilization/timeout; or
- Replace with a “recent weight sample” check.
Minimal change:
- bool canProcessWeight = (controller != nullptr); - if (canProcessWeight) { - canProcessWeight = controller->isVolumetricAvailable(); - } - - if (!canProcessWeight) { - // If BLE connection is unstable, end extended recording early - extendedRecording = false; - return; - } + // Rely on stabilization/timeout below; no volumetric-availability gate here.
🧹 Nitpick comments (1)
web/src/pages/ShotHistory/utils.js (1)
8-12: Guard header parsing (optional).Add a simple header length check to avoid NaN timestamp/profile if header is malformed.
Apply:
- const header = lines[0].split(','); + const header = (lines[0] || '').split(','); + if (header.length < 3) return null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/display/plugins/ShotHistoryPlugin.cpp(4 hunks)src/display/plugins/ShotHistoryPlugin.h(3 hunks)web/src/pages/ShotHistory/utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/display/plugins/ShotHistoryPlugin.h
⏰ 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). (2)
- GitHub Check: deploy
- GitHub Check: test
| void ShotHistoryPlugin::finalizeRecording() { | ||
| unsigned long duration = millis() - shotStart; | ||
| if (duration <= 7500) { // Exclude failed shots and flushes | ||
| SPIFFS.remove("/h/" + currentId + ".dat"); | ||
| } else { | ||
| controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); | ||
| cleanupHistory(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Also remove notes file for short shots.
Align finalizeRecording() behavior with the close path to avoid orphaned .json files.
Apply:
void ShotHistoryPlugin::finalizeRecording() {
unsigned long duration = millis() - shotStart;
if (duration <= 7500) { // Exclude failed shots and flushes
SPIFFS.remove("/h/" + currentId + ".dat");
+ SPIFFS.remove("/h/" + currentId + ".json");
} else {
controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1);
cleanupHistory();
}
}📝 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.
| void ShotHistoryPlugin::finalizeRecording() { | |
| unsigned long duration = millis() - shotStart; | |
| if (duration <= 7500) { // Exclude failed shots and flushes | |
| SPIFFS.remove("/h/" + currentId + ".dat"); | |
| } else { | |
| controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); | |
| cleanupHistory(); | |
| } | |
| } | |
| void ShotHistoryPlugin::finalizeRecording() { | |
| unsigned long duration = millis() - shotStart; | |
| if (duration <= 7500) { // Exclude failed shots and flushes | |
| SPIFFS.remove("/h/" + currentId + ".dat"); | |
| SPIFFS.remove("/h/" + currentId + ".json"); | |
| } else { | |
| controller->getSettings().setHistoryIndex(controller->getSettings().getHistoryIndex() + 1); | |
| cleanupHistory(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/display/plugins/ShotHistoryPlugin.cpp around lines 157 to 165,
finalizeRecording() currently removes only the .dat file for short/failed shots
and leaves the associated .json notes file orphaned; update the short-shot
branch to also remove the JSON notes file (e.g. SPIFFS.remove("/h/" + currentId
+ ".json")) so behavior matches the close path and both .dat and .json are
cleaned up for short recordings.
| if (data['samples'] && data['samples'].length > 0) { | ||
| const lastSample = data['samples'][data['samples'].length - 1]; | ||
| data.duration = lastSample.t; | ||
| data.volume = lastSample.v; | ||
| let finalVolume = maxVolume > 0 ? maxVolume : lastValidVolume; | ||
|
|
||
| // If we still don't have a valid volume, try the last sample's volume | ||
| if (finalVolume <= 0 && !isNaN(lastSample.v) && lastSample.v > 0) { | ||
| finalVolume = lastSample.v; | ||
| } else if (finalVolume <= 0 && !isNaN(lastSample.ev) && lastSample.ev > 0) { | ||
| finalVolume = lastSample.ev; | ||
| } | ||
|
|
||
| data.volume = finalVolume > 0 ? parseFloat(finalVolume.toFixed(1)) : null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Finish volume selection by using ev maxima when no valid v was seen.
Prevents null/low volume for legacy data.
Apply:
- const lastSample = data['samples'][data['samples'].length - 1];
+ const lastSample = data['samples'][data['samples'].length - 1];
data.duration = lastSample.t;
- let finalVolume = maxVolume > 0 ? maxVolume : lastValidVolume;
+ let finalVolume = maxVolume > 0 ? maxVolume : lastValidVolume;
- // If we still don't have a valid volume, try the last sample's volume
- if (finalVolume <= 0 && !isNaN(lastSample.v) && lastSample.v > 0) {
- finalVolume = lastSample.v;
- } else if (finalVolume <= 0 && !isNaN(lastSample.ev) && lastSample.ev > 0) {
- finalVolume = lastSample.ev;
- }
+ if (finalVolume <= 0) {
+ // If no valid 'v' was seen, use the best 'ev' we have
+ finalVolume = maxEv > 0 ? maxEv : lastValidEv;
+ }
+ // Still nothing? fall back to last sample
+ if (finalVolume <= 0 && !isNaN(lastSample.v) && lastSample.v > 0) {
+ finalVolume = lastSample.v;
+ } else if (finalVolume <= 0 && !isNaN(lastSample.ev) && lastSample.ev > 0) {
+ finalVolume = lastSample.ev;
+ }📝 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.
| if (data['samples'] && data['samples'].length > 0) { | |
| const lastSample = data['samples'][data['samples'].length - 1]; | |
| data.duration = lastSample.t; | |
| data.volume = lastSample.v; | |
| let finalVolume = maxVolume > 0 ? maxVolume : lastValidVolume; | |
| // If we still don't have a valid volume, try the last sample's volume | |
| if (finalVolume <= 0 && !isNaN(lastSample.v) && lastSample.v > 0) { | |
| finalVolume = lastSample.v; | |
| } else if (finalVolume <= 0 && !isNaN(lastSample.ev) && lastSample.ev > 0) { | |
| finalVolume = lastSample.ev; | |
| } | |
| data.volume = finalVolume > 0 ? parseFloat(finalVolume.toFixed(1)) : null; | |
| } | |
| if (data['samples'] && data['samples'].length > 0) { | |
| const lastSample = data['samples'][data['samples'].length - 1]; | |
| data.duration = lastSample.t; | |
| let finalVolume = maxVolume > 0 ? maxVolume : lastValidVolume; | |
| if (finalVolume <= 0) { | |
| // If no valid 'v' was seen, use the best 'ev' we have | |
| finalVolume = maxEv > 0 ? maxEv : lastValidEv; | |
| } | |
| // Still nothing? fall back to last sample | |
| if (finalVolume <= 0 && !isNaN(lastSample.v) && lastSample.v > 0) { | |
| finalVolume = lastSample.v; | |
| } else if (finalVolume <= 0 && !isNaN(lastSample.ev) && lastSample.ev > 0) { | |
| finalVolume = lastSample.ev; | |
| } | |
| data.volume = finalVolume > 0 ? parseFloat(finalVolume.toFixed(1)) : null; | |
| } |
🤖 Prompt for AI Agents
In web/src/pages/ShotHistory/utils.js around lines 68-81, the volume selection
currently falls back only to lastSample.v when maxVolume and lastValidVolume are
not >0, which leaves legacy entries with null/low volume; update the fallback
logic so that after checking lastSample.v you also check lastSample.ev (the
energy-validated/max value) and use it if positive, then set data.volume to that
value rounded to one decimal or null if still <=0; ensure finalVolume uses
maxVolume first, then lastValidVolume, then lastSample.v, then lastSample.ev
before assigning data.volume.
…r the shot is finished to allow all of the liquid to enter the cup before finalizing the shot data. If the volumetric mode is active, the shot recording will now wait for the weight to stabilize before finalizing the shot data. This prevents premature finalization when the weight is still changing due to liquid settling. This is limited to a maximum of 3 seconds after the shot is finished or until the weight has stabilized.
dragm83
force-pushed
the
shot_record_fix
branch
from
54753d3 to
b399c62
Compare
|
Fix for shot history not accounting for liquid still in transit after brew ends and recording an incorrect (premature) brew weight. This history will record for an additional 3 seconds OR until the weight stabilizes (<0.1g delta in 1s).
Summary by CodeRabbit
New Features
Bug Fixes
Refactor