Add model name to Rollbar; change API_URL to API_HOST (UPLOAD-1661, UPLOAD-421)#1754
Add model name to Rollbar; change API_URL to API_HOST (UPLOAD-1661, UPLOAD-421)#1754gniezen wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves upload error observability in Rollbar (better grouping + more device context) and renames the uploader’s API endpoint configuration from API_URL to API_HOST across runtime config, docker/dev tooling, and tests.
Changes:
- Add
deviceModel(“model”) into Rollbar custom payload and adjust Rollbar fingerprinting to group by errorcode. - Preserve
deviceModelthroughdriverManagerwaterfall errors so it remains available for error reporting/cleanup paths. - Rename configuration/env usage from
API_URL→API_HOSTacross app initialization, CLI tools, docker artifacts, and tests.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/app/actions/async.test.js | Updates async action tests to use API_HOST config key. |
| server.js | Updates CSP connectSrc to allow the configured API host via API_HOST. |
| package.json | Bumps version to a prerelease tag for this change set. |
| lib/drivers/onetouch/cli/otverio.js | Uses cfg.API_HOST when creating API client. |
| lib/drivers/medtronic600/cli/blob_loader.js | Uses cfg.API_HOST when creating API client. |
| lib/drivers/medtronic/cli/blob_loader.js | Uses config.API_HOST when creating API client. |
| lib/drivers/insulet/cli/ibf_loader.js | Uses cfg.API_HOST when creating API client. |
| lib/drivers/abbott/cli/fslibre.js | Uses cfg.API_HOST when creating API client. |
| lib/drivers/abbott/cli/csv_loader.js | Uses cfg.API_HOST when creating API client. |
| lib/driverManager.js | Tracks lastData so deviceModel can be preserved on waterfall error paths. |
| lib/core/api.js | Updates setHosts to look for API_HOST when setting Tidepool API host. |
| Dockerfile | Renames build args/env from API_URL to API_HOST. |
| docker-compose.yaml | Renames compose environment variable from API_URL to API_HOST. |
| config/local.sh | Renames local env export from API_URL to API_HOST. |
| config/local.example.js | Renames example env values from API_URL to API_HOST. |
| app/utils/rollbar.js | Sets Rollbar fingerprint by error code to improve grouping. |
| app/store/configureStore.production.js | Uses config.API_HOST for API client creation. |
| app/store/configureStore.development.js | Uses config.API_HOST for API client creation. |
| app/package.json | Keeps app version aligned with root package prerelease tag. |
| app/containers/App.js | Renames environment menu/server definitions to use API_HOST. |
| app/actions/utils.js | Adds model to Rollbar custom payload for upload errors. |
| app/actions/async.js | Uses API_HOST when initializing API hosts during app init. |
| .config.js | Renames exported config key and env lookup to API_HOST. |
Comments suppressed due to low confidence (1)
Dockerfile:64
- Same issue as the build stage: the production image now only exports
API_HOST, but other parts of the repo/tooling still referenceAPI_URL. Exporting both variables (and accepting both ARGs) avoids breakage for existing deploy tooling during the rename.
ARG API_HOST
ARG UPLOAD_URL
ARG DATA_URL
ARG BLIP_URL
ARG REALM_HOST
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DEBUG: stringToBoolean(process.env.DEBUG, true), | ||
| // the defaults for these need to be pointing to prod | ||
| API_URL: process.env.API_URL || serverEnvironments[selectedServerEnv].API_URL, | ||
| API_HOST: process.env.API_HOST || serverEnvironments[selectedServerEnv].API_HOST, |
| if (hosts.API_HOST) { | ||
| tidepool.setApiHost(hosts.API_HOST); | ||
| } |
| frameSrc: ['https://docs.google.com', 'https://app.pendo.io', '*.tidepool.org', 'localhost:*', 'tidepooluploader://*'], | ||
| connectSrc: [].concat([ | ||
| process.env.API_URL || 'localhost:*', | ||
| process.env.API_HOST || 'localhost:*', |
| ARG VERSION_SHA | ||
| ARG API_URL | ||
| ARG API_HOST | ||
| ARG UPLOAD_URL |
| result = result || {}; | ||
| if (result.deviceModel == null && lastData.deviceModel != null) { | ||
| result.deviceModel = lastData.deviceModel; | ||
| } |
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR renames the API_URL configuration key to API_HOST across build configs, Docker files, server CSP settings, app initialization, store setup, core API client, and CLI driver login flows. It also adds device model tracking through the upload pipeline, backfilling deviceModel on cleanup, includes it in upload error payloads, and adjusts Rollbar error fingerprinting to use error codes. Version numbers were bumped in package.json files. ChangesAPI_URL to API_HOST rename
Device model tracking and error fingerprinting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DriverManager as lib/driverManager.js
participant Driver as Device Driver
participant Cleanup as drvr.cleanup
DriverManager->>Driver: connect/fetchData/processData/uploadData(data)
Driver-->>DriverManager: returns data, stores as lastData
DriverManager->>DriverManager: check result.deviceModel missing
DriverManager->>DriverManager: backfill result.deviceModel from lastData.deviceModel
DriverManager->>Cleanup: cleanup(result)
Related PRs: None identified. Suggested labels: configuration, breaking-change Suggested reviewers: None identified. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)test/app/actions/async.test.jsast-grep timed out on this file 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: 2
🧹 Nitpick comments (1)
app/utils/rollbar.js (1)
36-48: 🧹 Nitpick | 🔵 TrivialFingerprint-by-code grouping is reasonable; matches existing payload access pattern in this file.
Direct
payload.custom/payload.fingerprintaccess is consistent with the existing directpayload.body.traceusage above (Line 27), so no structural mismatch. One tradeoff worth being aware of: grouping strictly bycustom.codewill collapse all occurrences sharing a generic code (e.g.E_SERVER_ERR) into a single Rollbar item regardless of device/model/message differences, which may hide distinct root causes behind one item. This appears intentional per the added comments, so no action needed unless that granularity loss becomes a problem in practice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/utils/rollbar.js` around lines 36 - 48, No code change is needed here: the direct payload.custom and payload.fingerprint access in rollbar.js is already consistent with the existing payload.body.trace handling, and the new fingerprint-by-code logic is intentional. If you later decide the grouping is too coarse, update the fingerprinting in the same rollbar payload path to include additional discriminators beyond payload.custom.code so distinct failure causes do not collapse into one Rollbar item.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/containers/App.js`:
- Line 176: The persisted server config upgrade path in App.js is missing a
legacy fallback for selectedEnv values saved with API_URL, so upgraded installs
can load the wrong API host while still reusing the other saved hosts. Update
the startup/config loading logic around api.setHosts and the selectedEnv
deserialization in App to treat API_URL as a fallback for API_HOST, and apply
the same fallback consistently in both places so old persisted configs map to
the intended host fields.
In `@lib/driverManager.js`:
- Around line 109-113: The debug-mode waterfall in driverManager.js bypasses the
local wrapper functions, so lastData is not updated for
connect/getConfigInfo/fetchData/processData/uploadData/disconnect and
result.deviceModel can’t be backfilled on error. Update the debug path to call
the same wrapper helpers (or otherwise assign lastData consistently) used in the
non-debug path, while keeping uploadBlob tracked too, so the final error handler
can reliably read the latest pipeline data.
---
Nitpick comments:
In `@app/utils/rollbar.js`:
- Around line 36-48: No code change is needed here: the direct payload.custom
and payload.fingerprint access in rollbar.js is already consistent with the
existing payload.body.trace handling, and the new fingerprint-by-code logic is
intentional. If you later decide the grouping is too coarse, update the
fingerprinting in the same rollbar payload path to include additional
discriminators beyond payload.custom.code so distinct failure causes do not
collapse into one Rollbar item.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: abf1f7e2-bba6-43ae-aa98-61f668f09b1e
📒 Files selected for processing (23)
.config.jsDockerfileapp/actions/async.jsapp/actions/utils.jsapp/containers/App.jsapp/package.jsonapp/store/configureStore.development.jsapp/store/configureStore.production.jsapp/utils/rollbar.jsconfig/local.example.jsconfig/local.shdocker-compose.yamllib/core/api.jslib/driverManager.jslib/drivers/abbott/cli/csv_loader.jslib/drivers/abbott/cli/fslibre.jslib/drivers/insulet/cli/ibf_loader.jslib/drivers/medtronic/cli/blob_loader.jslib/drivers/medtronic600/cli/blob_loader.jslib/drivers/onetouch/cli/otverio.jspackage.jsonserver.jstest/app/actions/async.test.js
| const url = `${protocol}${server.name}:${server.port}`; | ||
| serverdata[server.name] = { | ||
| API_URL: url, | ||
| API_HOST: url, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'app/containers/App.js' 'lib/core/api.js' | sed 's#^`#-` #'
printf '\n== Search selectedEnv / API_URL / API_HOST ==\n'
rg -n --hidden --no-ignore-vcs -S 'selectedEnv|API_URL|API_HOST' app lib | sed -n '1,240p'
printf '\n== App.js relevant slice ==\n'
sed -n '1,240p' app/containers/App.js
printf '\n== api.js relevant slice ==\n'
sed -n '1,220p' lib/core/api.jsRepository: tidepool-org/uploader
Length of output: 16296
Handle legacy API_URL in persisted server config.
selectedEnv saved before this rename can still deserialize with API_URL, but api.setHosts() only reads API_HOST. That leaves upgraded Electron installs on the default API host while reusing the old upload/data/blip hosts from storage. Mirror the same fallback in both startup reads.
Suggested fallback
let parsedEnv = JSON.parse(selectedEnv);
+ if (!parsedEnv.API_HOST && parsedEnv.API_URL) {
+ parsedEnv.API_HOST = parsedEnv.API_URL;
+ }
console.log('setting initial server from localstore:', parsedEnv.environment);
api.setHosts(parsedEnv);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/containers/App.js` at line 176, The persisted server config upgrade path
in App.js is missing a legacy fallback for selectedEnv values saved with
API_URL, so upgraded installs can load the wrong API host while still reusing
the other saved hosts. Update the startup/config loading logic around
api.setHosts and the selectedEnv deserialization in App to treat API_URL as a
fallback for API_HOST, and apply the same fallback consistently in both places
so old persisted configs map to the intended host fields.
There was a problem hiding this comment.
@gniezen do you think that it's worth addressing loading old stored data here?
| // async.waterfall only passes the data object to the final callback on | ||
| // success; on error it's dropped. We keep a reference to the latest data | ||
| // flowing through the pipeline so the device model and other metadata | ||
| // is still available to report on error. | ||
| var lastData = {}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
lastData backfill doesn't work in debug mode for most pipeline stages.
The debug-mode waterfall (Lines 237-244) binds drvr.connect, drvr.getConfigInfo, drvr.fetchData, drvr.processData, drvr.uploadData, drvr.disconnect directly, bypassing the local wrapper functions (connect, getConfigInfo, fetchData, processData, uploadData, disconnect) that set lastData. Only the shared uploadBlob step updates lastData in this path. As a result, if an error occurs in any stage other than uploadBlob while configs.debug === true, lastData still holds its initial {} (or a stale value from before uploadBlob ran), and the backfill at Lines 247-249 can't populate result.deviceModel. This defeats the stated purpose of the change ("we keep a reference to the latest data flowing through the pipeline ... available to report on error") for the debug path.
🔧 Proposed fix: wrap debug-mode steps to also track lastData without adding try/catch
+ function withLastData(fn) {
+ return function(data, next) {
+ lastData = data;
+ fn(data, next);
+ };
+ }
+
if (configs.debug === true) {
debug('DriverManager set up in debug mode: all non-device comms errors will throw in console.');
console.time('total time elapsed');
async.waterfall([
drvr.setup.bind(drvr, deviceInfo, stat.progressForStep(0)),
- drvr.connect.bind(drvr, stat.progressForStep(1)),
- drvr.getConfigInfo.bind(drvr, stat.progressForStep(2)),
- drvr.fetchData.bind(drvr, stat.progressForStep(3)),
+ withLastData(drvr.connect.bind(drvr, stat.progressForStep(1))),
+ withLastData(drvr.getConfigInfo.bind(drvr, stat.progressForStep(2))),
+ withLastData(drvr.fetchData.bind(drvr, stat.progressForStep(3))),
uploadBlob,
- drvr.processData.bind(drvr, stat.progressForStep(4)),
- drvr.uploadData.bind(drvr, stat.progressForStep(5)),
- drvr.disconnect.bind(drvr, stat.progressForStep(6))
+ withLastData(drvr.processData.bind(drvr, stat.progressForStep(4))),
+ withLastData(drvr.uploadData.bind(drvr, stat.progressForStep(5))),
+ withLastData(drvr.disconnect.bind(drvr, stat.progressForStep(6)))
], function(err, result) {Also applies to: 236-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/driverManager.js` around lines 109 - 113, The debug-mode waterfall in
driverManager.js bypasses the local wrapper functions, so lastData is not
updated for connect/getConfigInfo/fetchData/processData/uploadData/disconnect
and result.deviceModel can’t be backfilled on error. Update the debug path to
call the same wrapper helpers (or otherwise assign lastData consistently) used
in the non-debug path, while keeping uploadBlob tracked too, so the final error
handler can reliably read the latest pipeline data.
There was a problem hiding this comment.
@gniezen This seems like an actual concern for the context/bind behavior that would need to be adjusted to make sure the reference is correct for the debug cases.
|
@gniezen as the AI code reviews mention in a couple places, there are still API_URL references in the |
Addresses UPLOAD-1661 and UPLOAD-421.