Skip to content

ref(dashboards): Extract breakdown table matching and support multi-groupBy#111245

Merged
DominikB2014 merged 6 commits intomasterfrom
dominikbuszowiecki/dain-1387-convert-numeric-table-value-to-string
Mar 23, 2026
Merged

ref(dashboards): Extract breakdown table matching and support multi-groupBy#111245
DominikB2014 merged 6 commits intomasterfrom
dominikbuszowiecki/dain-1387-convert-numeric-table-value-to-string

Conversation

@DominikB2014
Copy link
Contributor

@DominikB2014 DominikB2014 commented Mar 20, 2026

Extract the inline logic that matches time series data to table rows in the breakdown legend into a dedicated matchTimeSeriesToTableRow function with tests.

Tested with following dashboards
image

The new implementation uses groupBy.every() to match all groupBy keys, enabling legend breakdown for widgets with multiple group-by columns. It also handles type coercion between numeric table values and string groupBy values (e.g., http.response_status_code: 200 in the table matching "200" from the backend) using Python's str() conversion rules.

Changes:

  • New matchTimeSeriestoTableRow.tsx with the extracted matching logic and toPythonString helper
  • Tests covering: no groupBy, single groupBy, multiple groupBy, numeric-to-string coercion, no match
  • Removed the widget builder restriction that disabled legend breakdown for >1 group-by column
  • Removed unused aggregates variable from visualizationWidget.tsx

Refs DAIN-1387

…roupBy

Extract the logic that matches time series data to table rows into a
dedicated `matchTimeSeriesToTableRow` function. The new implementation
uses `groupBy.every()` to match all groupBy keys, supporting multiple
group-by columns instead of just one. It also handles type coercion
between numeric table values and string groupBy values using Python's
str() conversion rules.

Remove the widget builder restriction that disabled legend breakdown
for widgets with more than one group-by column.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@linear-code
Copy link

linear-code bot commented Mar 20, 2026

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 20, 2026
@DominikB2014 DominikB2014 marked this pull request as ready for review March 20, 2026 20:19
@DominikB2014 DominikB2014 requested a review from a team as a code owner March 20, 2026 20:19
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: toPythonString mishandles null as 'null' not 'None'
    • Added explicit null check that returns 'None' to match Python's str(None) output, ensuring table row matching works correctly for null groupBy values.
  • ✅ Fixed: Array conversion in toPythonString doesn't match Python
    • Updated array formatting to match Python's str([...]) by adding spaces after commas, wrapping strings in quotes, and converting null elements to 'None'.

Create PR

Or push these changes by commenting:

@cursor push a7e8608010
Preview (a7e8608010)
diff --git a/static/app/views/dashboards/widgetCard/matchTimeSeriesToTableRow.spec.tsx b/static/app/views/dashboards/widgetCard/matchTimeSeriesToTableRow.spec.tsx
--- a/static/app/views/dashboards/widgetCard/matchTimeSeriesToTableRow.spec.tsx
+++ b/static/app/views/dashboards/widgetCard/matchTimeSeriesToTableRow.spec.tsx
@@ -90,4 +90,64 @@
 
     expect(result).toBe(42);
   });
+
+  it('matches null groupBy values correctly', () => {
+    const result = matchTimeSeriesToTableRow({
+      tableDataRows: [
+        {id: '1', project: 'None', 'count()': 15},
+        {id: '2', project: 'my-project', 'count()': 25},
+      ],
+      timeSeries: TimeSeriesFixture({
+        yAxis: 'count()',
+        groupBy: [{key: 'project', value: null}],
+      }),
+    });
+
+    expect(result).toBe(15);
+  });
+
+  it('matches array groupBy values with strings correctly', () => {
+    const result = matchTimeSeriesToTableRow({
+      tableDataRows: [
+        {id: '1', 'error.type': "['ValueError', 'TypeError']", 'count()': 10},
+        {id: '2', 'error.type': "['Exception']", 'count()': 5},
+      ],
+      timeSeries: TimeSeriesFixture({
+        yAxis: 'count()',
+        groupBy: [{key: 'error.type', value: ['ValueError', 'TypeError']}],
+      }),
+    });
+
+    expect(result).toBe(10);
+  });
+
+  it('matches array groupBy values with null elements correctly', () => {
+    const result = matchTimeSeriesToTableRow({
+      tableDataRows: [
+        {id: '1', 'error.type': "['Exception', None, 'TypeError']", 'count()': 20},
+        {id: '2', 'error.type': "['ValueError']", 'count()': 8},
+      ],
+      timeSeries: TimeSeriesFixture({
+        yAxis: 'count()',
+        groupBy: [{key: 'error.type', value: ['Exception', null, 'TypeError']}],
+      }),
+    });
+
+    expect(result).toBe(20);
+  });
+
+  it('matches array groupBy values with numbers correctly', () => {
+    const result = matchTimeSeriesToTableRow({
+      tableDataRows: [
+        {id: '1', 'status_codes': '[200, 404]', 'count()': 30},
+        {id: '2', 'status_codes': '[500]', 'count()': 2},
+      ],
+      timeSeries: TimeSeriesFixture({
+        yAxis: 'count()',
+        groupBy: [{key: 'status_codes', value: [200, 404]}],
+      }),
+    });
+
+    expect(result).toBe(30);
+  });
 });

diff --git a/static/app/views/dashboards/widgetCard/matchTimeSeriestoTableRow.tsx b/static/app/views/dashboards/widgetCard/matchTimeSeriestoTableRow.tsx
--- a/static/app/views/dashboards/widgetCard/matchTimeSeriestoTableRow.tsx
+++ b/static/app/views/dashboards/widgetCard/matchTimeSeriestoTableRow.tsx
@@ -7,12 +7,36 @@
  * match that behavior for comparisons.
  */
 function toPythonString(value: unknown): string {
+  if (value === null) {
+    return 'None';
+  }
   if (typeof value === 'boolean') {
     return value ? 'True' : 'False';
   }
   if (Array.isArray(value)) {
-    return `[${value.join(',')}]`;
+    const elements = value.map(item => {
+      if (item === null) {
+        return 'None';
+      }
+      if (typeof item === 'string') {
+        return `'${item}'`;
+      }
+      if (typeof item === 'number') {
+        return item.toString();
+      }
+      // Fallback for unexpected types in array
+      return String(item);
+    });
+    return `[${elements.join(', ')}]`;
   }
+  if (typeof value === 'string') {
+    return value;
+  }
+  if (typeof value === 'number') {
+    return value.toString();
+  }
+  // Fallback for unexpected types (should never be reached with valid GroupBy values)
+  // eslint-disable-next-line @typescript-eslint/no-base-to-string
   return String(value);
 }

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

The previous array conversion used value.join(',') which produced
[a,b], but Python's str() on a list produces ['a', 'b'] with spaces
after commas, quotes around strings, and None for null elements.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

Cool! Just two small comments

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@DominikB2014 DominikB2014 requested a review from a team as a code owner March 23, 2026 14:16
Better reflects the return type — the function returns the matched
aggregate value, not the row itself.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Value

Update import paths in the spec file and visualizationWidget to
reference the renamed module with corrected casing.

Co-Authored-By: Claude Opus 4.6 <noreply@example.com>
@DominikB2014 DominikB2014 requested a review from gggritso March 23, 2026 14:22
@DominikB2014
Copy link
Contributor Author

@gggritso renamed the function name and moved toPythonString to a util file!

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

});
return `[${items.join(', ')}]`;
}
return String(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

toPythonString mishandles top-level null and undefined

Medium Severity

toPythonString converts null/undefined inside arrays to 'None' but falls through to String(null)"null" at the top level, whereas Python's str(None) returns "None". The GroupBy type allows value: string | null | ..., so when a groupBy value is null, toPythonString(null) produces "null", which won't match the table row's "None" string from the backend, causing the breakdown legend to fail to find the matching row.

Additional Locations (1)
Fix in Cursor Fix in Web

Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

That Bugbot comment is worth addressing IMO, but not blocking for this PR

@DominikB2014 DominikB2014 merged commit 74a116c into master Mar 23, 2026
70 checks passed
@DominikB2014 DominikB2014 deleted the dominikbuszowiecki/dain-1387-convert-numeric-table-value-to-string branch March 23, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants