[WEB-3632] - Add Device Names to PDF Export by henry-tp · Pull Request #586 · tidepool-org/viz

henry-tp · 2026-01-22T23:37:43Z

henry-tp · 2026-01-28T19:44:04Z

+
+    const isPageBreakAtTableStart = !_.isNil(tableLabel) && _.isNil(tableData);
+
+    if (isPageBreakAtTableStart) return; // prevent double header on new page


An issue occurs where if the first page has content all the way to the bottom, this table needs to break the page but does not have enough room to print even one line of data before breaking, it will start on the next page with a duplicate header.

Before and After:

Nice catch 👍

Copilot

Pull request overview

This pull request adds device name information to PDF exports in the Basics view, addressing ticket WEB-3632. The implementation extracts device names from upload data, provides a utility function to retrieve user-friendly device names with fallback logic, and renders them in a dedicated table section on the PDF.

Changes:

Added deviceName property to the Upload data type and related data flow
Implemented getDeviceName() utility function to retrieve render-friendly device names with fallback to labels
Added device names rendering section in the Basics PDF print view with automatic height calculation for line wrapping
Enhanced onPageAdded handler to prevent duplicate headers when page breaks occur at table boundaries
Updated test fixtures with device name data

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
data/types.js	Added deviceName property to Upload class
src/utils/DataUtil.js	Extracted and stored deviceName from upload data in device objects
src/utils/device.js	Added getDeviceName utility function with fallback logic from deviceName to label
src/modules/print/PrintView.js	Added devices property extraction and improved onPageAdded handler to prevent double headers
src/modules/print/BasicsPrintView.js	Implemented renderDeviceNames method to display unique device names in PDF
test/utils/DataUtil.test.js	Updated test fixtures and assertions to include deviceName property

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

clintonium-119

I have a question about the rendering approach in the basics print view that needs addressing.

Also, I'm not seeing the device list additons to the bgLog and daily print views, though they seem to be required by the Jira ticket.

clintonium-119 · 2026-01-29T16:09:54Z

    });
  });
+
+  describe('getDeviceName', () => {


Small nit - could add a check to make sure 'Unknown' is passed over as in the utility function for completeness

clintonium-119 · 2026-01-29T16:17:27Z

+
+    if (!deviceNames.length) return;
+
+    const rows = [{ label: deviceNames.join('\n\n') }];


This approach seems strange to me, combining all of the devices into a single table row and then having to calculate the heights, account for double-spacing, etc.

Is there a reason you didn't just have each one be a row, and then let the table work this all out natively?

I believe you should be able to set the table styling to match the figma for this, while using rows in a more idiomatic way.

Okay, I was super close to giving up but managed to do it that way. Significant brute force may have been involved 8)

Prior to today, I was not able to make this work, because despite whatever text I put into the box, the container would not dynamically scale to the height of the text - it only added about one line's worth of height

Explanation of changes:

Per my understanding, the voilab-pdf-table library calls the renderer function twice - first with draw = false. The returned value from this call is used for measuring of the contents so the container can be drawn correctly. The second call with draw = true is then used for the actual drawing.

Prior to my changes to the custom renderer function, it would always return ' ' on the first call. This meant it was measuring the height of a whitespace (for a single line) no matter the input text.

Now, I have adjusted it to return the text itself on the first call. This means it will actually measure to get the necessary height of the container.

With due respect to the library creators, this was confusing as heck

clintonium-119 · 2026-01-29T16:19:09Z

+
+    const isPageBreakAtTableStart = !_.isNil(tableLabel) && _.isNil(tableData);
+
+    if (isPageBreakAtTableStart) return; // prevent double header on new page


Nice catch 👍

…t multiple times

henry-tp · 2026-02-03T03:47:53Z

      topEdge: this.margins.top,
    };

+    this.deviceNamesHeader = null; // if needed, call this.generateDeviceNamesHeader();


I wanted the deviceNameHeader property to be shared logic, but I didn't want it to be initialized in reports that don't need it (e.g. if user is only printing AGP). This is just a funky little pattern so that we are only generating the deviceNameHeader data when it is needed, by calling it in the subclasses that use it

A better way to do this might be defining a fun getter property like

get deviceNamesHeader () { if (!deviceNamesHeaderData) { this.deviceNamesHeaderData = this.generateDeviceNamesHeader(); } return this.deviceNamesHeaderData; }

but we don't use any getter properties throughout our codebase so I didn't go with that

I'm fine with either approach.

henry-tp · 2026-02-03T03:56:21Z

@@ -498,18 +503,30 @@ class PrintView {
  }

  renderCustomTextCell(tb, data, draw, column, pos, padding, isHeader) {


Previously, in the pre-draw phase where the container sizes were measured, we were always returning ' '. This meant that we were always measuring the container to fit the height needed for a single whitespace character. We could override this by passing a number for the height, but if left to calculate the necessary height dynamically, voilab-pdf-table would measure based on the whitespace character. This is fine for single-line text cells (which is all we had before) but now, with dynamic-height text cells, we need to give the PDF library the actual string so it can dynamically set the height of the container.

henry-tp · 2026-02-03T04:26:20Z

+      label,
+      content,
+      height,
+    };


Unfortunately this is more complicated than I wanted it to be with all of the measurements. Per the requirements, the height needed to be dynamic to accommodate large device lists. This ensures that the daily and bgLog charts can adjust their content start positions based on the list length, like so.

Has any product discussion come up about whether or not we should be filtering the devices list to only display those present in the data used for a given view?

It would seem to me to be ideal to only list BGM devices on this view, in particular.
But if you did go to the trouble of filtering, it's probably not too time-consuming to also filter the daily and basics device lists too.

Thanks @clintonium-119 , I don't know why this eluded me or I didn't think about this.

I made a discussion thread here: https://tidepoolteam.slack.com/archives/C01ELEQUVRD/p1772666366797099

The way I see it, there are two sub-questions:

A) Do we filter only for devices for the patient that have data in the current time frame?

B) Do we only show BGM devices on BgLog report pages?

For Question A, if we do want to filter only for those, my intuition is to filter the devices array by looking to see if it exists in matchedDevices.

In your opinion, do you think that approach would work for Question A?

@clintonium-119 okay, so the direction from product (slack thread):

Show all devices (regardless of bgm/cgm) but only those with data for the current time frame.

I've made the change to match those with matchedDevices here: 6380d15

henry-tp · 2026-02-04T17:52:59Z

@clintonium-119 I have revamped the entire PR. Please have another look, but code that you "already reviewed" may have been changed significantly.

clintonium-119

This looks good, but I'd just like confirmation that the product intent is not to show the a filtered device list for each report section, so that, for instance, the BG Log view doesn't list CGMs and pumps that did not provide bg reading for that view, and so on.

clintonium-119 · 2026-02-19T15:39:47Z

      topEdge: this.margins.top,
    };

+    this.deviceNamesHeader = null; // if needed, call this.generateDeviceNamesHeader();


I'm fine with either approach.

clintonium-119 · 2026-03-04T21:08:28Z

+      label,
+      content,
+      height,
+    };


Has any product discussion come up about whether or not we should be filtering the devices list to only display those present in the data used for a given view?

It would seem to me to be ideal to only list BGM devices on this view, in particular.
But if you did go to the trouble of filtering, it's probably not too time-consuming to also filter the daily and basics device lists too.

henry-tp · 2026-03-09T21:12:54Z

@clintonium-119 ready for another look. Added filtering by matchedDevices

henry-tp · 2026-04-03T01:06:21Z

Hey @clintonium-119 , this is ready for another look

clintonium-119

LGTM 🔥

henry-tp added 8 commits January 22, 2026 15:34

WEB-3632 extract deviceNames from metaData

9ba8a39

WEB-3632 adjust column height correctly based on line count

05fc30c

WEB-3632 improve code organization in renderDeviceNames()

6d82de7

WEB-3632 fix lint

67fddaa

WEB-3632 do not render box when no devices

819fbab

WEB-3632 fix tests

136a79d

WEB-3632 resolve cases of duplicated devices

744f663

WEB-3632 prevent doubled headers

91ecec0

henry-tp commented Jan 28, 2026

View reviewed changes

WEB-3632 abstract deviceName

a90de2b

henry-tp marked this pull request as ready for review January 28, 2026 20:16

krystophv requested a review from Copilot January 28, 2026 20:28

Copilot started reviewing on behalf of krystophv January 28, 2026 20:28 View session

Copilot AI reviewed Jan 28, 2026

View reviewed changes

henry-tp added 3 commits January 28, 2026 12:59

WEB-3632 fix tests

80c50d3

WEB-3632 fix jsdoc arg

9700db3

v1.55.0-web-3632-device-names.1

3f7ac87

henry-tp mentioned this pull request Jan 28, 2026

[WEB-3632] - Add Device Names to PDF Export tidepool-org/blip#1851

Open

henry-tp added 6 commits January 28, 2026 14:53

WEB-3632 update copy-as-text device names

2a05665

WEB-3632 export getDeviceName fn

5451911

v1.55.0-web-3632-device-names.2

1f05761

WEB-3632 fix lint

9750b7e

WEB-3632 add new tests

5d74333

v1.55.0-web-3632-device-names.3

6fe84af

henry-tp requested a review from clintonium-119 January 28, 2026 23:25

henry-tp added 4 commits January 28, 2026 15:36

WEB-3632 add test for getDeviceName

94f41f7

v1.55.0-web-3632-device-names.4

a7a5f8a

Merge branch 'develop' into WEB-3632-device-names

5d76ffe

v1.55.0-web-3632-device-names.5

17a1f67

clintonium-119 requested changes Jan 29, 2026

View reviewed changes

henry-tp added 15 commits January 29, 2026 14:51

WEB-3632 modify renderCustomTextCell to allow for dynamicHeight

b13aea9

WEB-3632 add comment to clarify measure behaviour

cb39278

WEB-3632 add test case for Unknown devices

e250622

WEB-3632 add hasDynamicHeight flag to be sure that it is intended

3a73285

WEB-3632 add device list to bgLog

cb901e0

WEB-3632 add Device List to bgLog and daily printouts

c6b1e2d

WEB-3632 position device names header above bgCharts

d681e01

WEB-3632 renderDeviceNamesHeader adjusts doc.y position after render

3733185

WEB-3632 remove unused deviceNamesHeaderHeight var

0dc83ff

WEB-3632 set deviceNamesHeader to object instead of calculating heigh…

959bcfa

…t multiple times

WEB-3632 singular name if only 1 device

0d71e79

WEB-3632 fix tests

cdb0ff3

WEB-3632 update SettingsPrintView device manufacturer label

2e625ab

WEB-3632 move getDeviceNames to abstracted util fn

6b02845

WEB-3632 generate devicenameheader only when needed

959ad24

henry-tp commented Feb 3, 2026

View reviewed changes

v1.55.0-web-3632-device-names.6

cfb810f

henry-tp requested a review from clintonium-119 February 3, 2026 04:08

henry-tp commented Feb 3, 2026

View reviewed changes

henry-tp added 2 commits February 2, 2026 21:55

WEB-3632 use existing loop construct to render header once on Daily view

5bdbe86

v1.55.0-web-3632-device-names.7

913aec2

clintonium-119 reviewed Mar 4, 2026

View reviewed changes

henry-tp added 2 commits March 9, 2026 13:31

WEB-3632 only show report devices

6380d15

v1.55.0-web-3632-device-names.8

4990036

henry-tp requested a review from clintonium-119 March 9, 2026 21:12

clintonium-119 approved these changes Apr 13, 2026

View reviewed changes


		const isPageBreakAtTableStart = !_.isNil(tableLabel) && _.isNil(tableData);

		if (isPageBreakAtTableStart) return; // prevent double header on new page


		if (!deviceNames.length) return;

		const rows = [{ label: deviceNames.join('\n\n') }];

		@@ -498,18 +503,30 @@ class PrintView {
		}

		renderCustomTextCell(tb, data, draw, column, pos, padding, isHeader) {

Uh oh!

Conversation

henry-tp commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henry-tp Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clintonium-119 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henry-tp Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henry-tp Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henry-tp commented Feb 4, 2026

Uh oh!

clintonium-119 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henry-tp commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henry-tp commented Apr 3, 2026

Uh oh!

clintonium-119 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

henry-tp commented Jan 22, 2026 •

edited

Loading

henry-tp Jan 28, 2026 •

edited

Loading

henry-tp Jan 29, 2026 •

edited

Loading

henry-tp Feb 3, 2026 •

edited

Loading

henry-tp commented Mar 9, 2026 •

edited

Loading