Skip to content

Conversation

@ewollesen
Copy link

Previously the code would add a number of minutes equal to the localTimezone. This is not correct, as JS stores dates in UTC. Adding the offset therefore did not adjust the timezone that's displayed, but rather changed the actual time value.

Since the offset is supplied, clients wishing to calculate the local time can do so on their own.

BACK-4205

@ewollesen
Copy link
Author

I had previously written #25 but later learned that the Jira ticket was out of date and that the preferred solution was to remove the localTime field entirely. This PR should accomplish that.

Previously the code would add a number of minutes equal to the
localTimezone. This is not correct, as JS stores dates in UTC. Adding
the offset therefore did not adjust the timezone that's displayed, but
rather changed the actual time value.

Since the offset is supplied, clients wishing to calculate the local
time can do so on their own.

BACK-4205
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the synthesized localTime field from Tidepool export processing and the XLSX field configuration, based on the fact that the prior offset-based calculation was mutating the actual timestamp rather than changing display timezone.

Changes:

  • Removed TidepoolDataTools.addLocalTime() and stopped synthesizing localTime in tidepoolProcessor.
  • Removed the localTime field from the export configuration so it no longer appears as an exported column.
  • Updated exporter comparison logic/tests to stop adding localTime, and bumped package version.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
index.js Removes the addLocalTime helper and stops injecting localTime into processed records.
config.json Drops localTime from exported field lists across data types, removing the column from outputs.
test/exporter.test.js Updates test normalization to no longer synthesize localTime before comparisons.
package.json Bumps package version for the change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{
"name": "@tidepool/data-tools",
"version": "2.4.2",
"version": "2.4.3",
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of the localTime field changes the exported schema (XLSX/flattened output), which is a backwards-incompatible change for consumers expecting that column/field. Consider bumping the package version accordingly (minor/major per your semver policy) and/or documenting the breaking change in release notes, rather than a patch-only bump.

Suggested change
"version": "2.4.3",
"version": "3.0.0",

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant