Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Bluetooth driver path to support uploading data from the Keto-Mojo GK+/GKI meter within the Tidepool Uploader device ecosystem.
Changes:
- Introduces a new Keto-Mojo BLE driver implementation (scan/connect, record retrieval, time set, record processing).
- Adds CRC-16/MODBUS support used by the new BLE protocol.
- Wires the new device into manifests/UI selection and adds a device checklist doc.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updates app version string for this build line. |
| app/package.json | Updates Electron app version string for this build line. |
| locales/en/translation.missing.json | Adds an English instruction string entry (currently in “missing” file). |
| lib/drivers/vivachek/ketomojo.js | New Keto-Mojo BLE driver and upload pipeline integration. |
| lib/crc.js | Adds CRC-16/MODBUS calculator used by the Keto-Mojo protocol. |
| lib/core/driverManifests.js | Registers KetoMojo driver as bluetooth mode. |
| lib/core/device.js | Registers KetoMojo driver and BLE comms mapping. |
| docs/checklists/ketomojo.md | Adds a device implementation checklist for Keto-Mojo. |
| app/reducers/devices.js | Adds Keto-Mojo device entry for UI selection/instructions. |
| app/components/Upload.js | Routes Keto-Mojo uploads through a dedicated BLE implementation instance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| { | ||
| "name": "tidepool-uploader", | ||
| "version": "2.65.0", | ||
| "version": "2.64.1-keto-mojo.5", |
There was a problem hiding this comment.
The version is being downgraded from 2.65.0 to 2.64.1-* (a lower SemVer). This can break update/packaging flows that assume monotonically increasing versions; consider bumping from 2.65.0 (e.g., 2.65.1-keto-mojo.5) or otherwise keeping the base version >= the current release line.
| "name": "tidepool-uploader", | ||
| "productName": "tidepool-uploader", | ||
| "version": "2.65.0", | ||
| "version": "2.64.1-keto-mojo.5", |
There was a problem hiding this comment.
The app package version is being downgraded from 2.65.0 to 2.64.1-* (lower SemVer). If Electron auto-update/build tooling relies on increasing versions, this can cause unexpected update behavior; consider bumping from 2.65.0 while still using a pre-release/build suffix.
| "For security reasons, we automatically sign you out after a certain period of inactivity, or if you've signed out from another browser tab.": "For security reasons, we automatically sign you out after a certain period of inactivity, or if you've signed out from another browser tab.", | ||
| "Please sign in again to continue.": "Please sign in again to continue.", | ||
| "Return to Login": "Return to Login", | ||
| "Turn meter on and check the Bluetooth icon is flashing": "Turn meter on and check the Bluetooth icon is flashing", |
There was a problem hiding this comment.
This repo’s i18n configuration only loads locales/*/translation.json (not translation.missing.json). Adding the new instruction string here won’t make it available for localization (e.g., Spanish) and is likely just noise; add the key to locales/en/translation.json (and locales/es/translation.json if applicable) instead, or rely on the key fallback without updating translation.missing.json manually.
| async connectTimeout(timeout = 40000) { | ||
| await Promise.race([ | ||
| this.connect(), | ||
| KetoMojo.timeout(timeout), | ||
| ]).catch((err) => { | ||
| debug('Error:', err); | ||
| self.dispatchEvent(new ErrorEvent('KetoMojoError', { | ||
| error: err, | ||
| })); | ||
| }); |
There was a problem hiding this comment.
connectTimeout() catches and dispatches the error but then resolves successfully (the catch handler doesn’t rethrow/return a rejected promise). The driver’s connect() step relies on connectTimeout() rejecting to hit its .catch and fail the upload early; as written, connection failures/timeouts can incorrectly proceed to later steps with a disconnected/partially initialized BLE state.
There was a problem hiding this comment.
@gniezen I think this is correct - the callback in the catch for the driver connect won't get called with the error unless we reject/rethrow here, right?
There was a problem hiding this comment.
Yeah, the way it's currently written doesn't look quite right - unnecessarily confusing at least.
| - `[x]` legacy "jellyfish" ingestion API | ||
| - `[ ]` platform ingestion API | ||
|
|
There was a problem hiding this comment.
The checklist marks legacy "jellyfish" ingestion API as selected, but the driver implementation uploads via cfg.api.upload.toPlatform(...). Update the checklist to reflect the actual ingestion path so it stays accurate for future maintenance.
|
@krystophv Please re-review - I've made the suggested changes. |
Addresses UPLOAD-1523.