Skip to content

properly adjust localtime according to localTimezone, if present #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ewollesen wants to merge 1 commit into
base: master
Choose a base branch
from
Open

properly adjust localtime according to localTimezone, if present #25

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ export default class TidepoolDataTools {

static addLocalTime(data) {
if (data.time) {
const localTime = new Date(data.time);
localTime.setUTCMinutes(localTime.getUTCMinutes() + (data.timezoneOffset || 0));
const offset = data.timezoneOffset || 0;
const localTime = moment(data.time).utcOffset(offset).format();
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

addLocalTime now sets localTime to a string (moment(...).format()), but the XLSX writer config treats localTime as a date field (cellFormat in config.json) and relies on ExcelJS receiving a Date to apply numFmt. As-is, localTime will be written as text in XLSX (formatting/sorting as dates won’t work). Consider either (a) keeping localTime as a Date for XLSX output (e.g., convert it to a Date in xlsxStreamWriter similarly to time/deviceTime/computerTime), or (b) if the intent is to preserve the timezone offset in the exported value, update the XLSX path/config to explicitly treat localTime as text and use a consistent string format.

Suggested change
const localTime = moment(data.time).utcOffset(offset).format();
const localMoment = moment(data.time).utcOffset(offset);
const localTime = localMoment.toDate();

Copilot uses AI. Check for mistakes.
Copy link
Member

@krystophv krystophv Feb 10, 2026

Choose a reason for hiding this comment

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

@ewollesen This comment is largely correct. The call stack in here gets pretty convoluted:

Example Flow for localTime
Config defines: "localTime": { "cellFormat": "yyyy-mm-dd hh:mm:ss" } (config.json)
Cache stores: cellFormat['smbg']['localTime'] = "yyyy-mm-dd hh:mm:ss" (TidepoolDataTools.cache)
Processing: addLocalTime() creates a JavaScript Date object
Column setup: cellFormat('smbg', 'localTime', { bgUnits: 'mmol/L' }) returns the Excel format string
ExcelJS applies the numFmt to format the Date when writing the cell

So currently the rest of the processing is expecting that value to be a Date() and not a string and may need to be adjusted accordingly (in the config) if we want to move over to a preformatted string from the Date.

Copy link
Author

@ewollesen ewollesen Feb 12, 2026

Choose a reason for hiding this comment

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

@krystophv based on Slack discussion that I wasn't aware of, I've updated the Jira ticket and posted a new PR (#26).

_.assign(data, {
localTime,
});
Expand Down
Loading