Clean up dashboard card types #38444

kulyk · 2024-02-05T17:03:30Z

A few changes around the DashboardCard type which should make it easier to write code handling different kinds of dashboard cards.

There're two main changes:

DashboardCard is now split into QuestionDashboardCard and VirtualDashboardCard type. The old code was basically combining the two, making some type checks awkward.
DashboardCard is now ActionDashboardCard | QuestionDashboardCard | VirtualDashboardCard, which should force us to narrow down the dashcards types while working with them (there are type guards)

The rest is fixing existing code to pass the type check

To verify: CI should be green

github-actions · 2024-02-05T17:04:39Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff ff09242...1ed9a9a.

Notify	File(s)
@alxnddr	frontend/src/metabase/visualizations/components/ObjectDetail/types.ts frontend/src/metabase/visualizations/index.ts frontend/src/metabase/visualizations/visualizations/Heading/Heading.tsx frontend/src/metabase/visualizations/visualizations/Heading/Heading.unit.spec.tsx frontend/src/metabase/visualizations/visualizations/LinkViz/LinkViz.tsx frontend/src/metabase/visualizations/visualizations/LinkViz/LinkViz.unit.spec.tsx

replay-io · 2024-02-05T17:15:21Z

Status	Complete ↗︎
Commit	`1ed9a9a`
Results	⚠️ 4 Flaky ✅ 2268 Passed

kamilmielnik

Amazing work 👏

kamilmielnik · 2024-02-06T14:22:08Z

frontend/src/metabase/dashboard/utils.ts

+): dashcard is QuestionDashboardCard {
  return "card_id" in dashcard && "card" in dashcard;


This type guard will return true also for ActionDashboardCard and VirtualDashboardCard - I doin't think we want that.

Suggested change

): dashcard is QuestionDashboardCard {

return "card_id" in dashcard && "card" in dashcard;

): dashcard is QuestionDashboardCard {

return

"card_id" in dashcard &&

"card" in dashcard &&

!isActionDashCard(dashcard) &&

!isVirtualDashCard(dashcard);

Or do we need a less specific type-guard, i.e. isDashboardCard(dashcard): dashcard is DashboardCard?

Good call, fixed in 0f8540c

kamilmielnik · 2024-02-06T14:25:26Z

frontend/src/metabase-types/api/dashboard.ts

@@ -58,7 +64,6 @@ export type BaseDashboardCard = {
  visualization_settings?: {
    [key: string]: unknown;
    virtual_card?: VirtualCard;
-    link?: LinkCardSettings;


this comment is actually for the line above this one

Should we remove virtual_card from BaseDashboardCard["visualization_settings"]?

I'm not yet sure how valuable it would be; it's actually quite convenient for some generic code, like here
I'd keep it for now, and we can always follow up on that later

some generic code, like here

Actually this is a good place to use isVirtualDashCard type-guard.

kamilmielnik · 2024-02-07T07:25:23Z

frontend/src/metabase/dashboard/components/DashboardGrid.tsx

@@ -292,7 +292,7 @@ class DashboardGrid extends Component<DashboardGridProps, DashboardGridState> {

  getLayoutForDashCard = (dashcard: BaseDashboardCard) => {
    const visualization = getVisualizationRaw([
-      { card: (dashcard as DashboardCard).card } as SingleSeries,
+      { card: (dashcard as QuestionDashboardCard).card } as SingleSeries,


Can we avoid the first type cast by using a type-guard?

Alternatively we should be able to use DashboardCard instead of BaseDashboardCard throughout this file.

Fixed in 61c17a7

kamilmielnik · 2024-02-07T07:33:44Z

frontend/src/metabase/parameters/utils/dashboards.ts

@@ -114,7 +114,7 @@ export function getDashboardUiParameters(
  metadata: Metadata,
 ): UiParameter[] {
  const { parameters, dashcards } = dashboard;
-  const mappings = getMappings(dashcards as DashboardCard[]);
+  const mappings = getMappings(dashcards as QuestionDashboardCard[]);


We can filter out dashcards using a type-guard to avoid the cast.
Same goes for hasMatchingParameters below.

Fixed in bcd3dc2

kamilmielnik · 2024-02-07T07:39:46Z

frontend/src/metabase/dashboard/actions/auto-wire-parameters/utils.ts

-  return cards.filter(dashcard => {
-    return (
+  return dashCards.filter(
+    dashcard =>


We can replace the cast with this:

Suggested change

dashcard =>

(dashcard): dashcard is QuestionDashboardCard =>

Fixed in 1ed9a9a

kamilmielnik · 2024-02-07T07:46:09Z

frontend/src/metabase/dashboard/components/DashCard/DashCard.tsx

        dashcard.visualization_settings,
      ),
    }),
    [dashcard],
  );

  const cards = useMemo(() => {
-    if (Array.isArray(dashcard.series)) {
+    if ("series" in dashcard && Array.isArray(dashcard.series)) {


We can use a type guard here:

Suggested change

if ("series" in dashcard && Array.isArray(dashcard.series)) {

if (isQuestionDashCard(dashcard)) {

Fixed in 5e7d3eb

kulyk · 2024-02-07T10:00:55Z

Thanks for the review @kamilmielnik 🙏
I've applied all of your suggestions, please take another look

kamilmielnik

🥇

github-actions · 2024-02-07T10:43:41Z

@kulyk Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

kulyk added no-backport Do not backport this PR to any branch .Team/DashViz Dashboard and Viz team labels Feb 5, 2024

kulyk self-assigned this Feb 5, 2024

metabase-bot bot added the visual Run Percy visual testing label Feb 5, 2024

kulyk changed the title ~~🚧 WIP — Clean up dashboard card types~~ Clean up dashboard card types Feb 5, 2024

kulyk requested review from kamilmielnik, alxnddr, uladzimirdev and a team February 5, 2024 18:24

kulyk force-pushed the virtual-dashcard-types branch from dea3ba5 to bc0b30a Compare February 5, 2024 18:28

kulyk added 5 commits February 6, 2024 02:03

Add standalone VirtualDashboardCard type

0d00f11

Rename DashboardCard to QuestionDashboardCard

dbef4c5

Add new DashboardCard type

0a686c4

Move ActionDashboardCard type def to dashboard types file

c808a24

Rename isQuestionDashCard type guard

08abf25

kulyk force-pushed the virtual-dashcard-types branch from bc0b30a to 08abf25 Compare February 6, 2024 02:03

kamilmielnik reviewed Feb 7, 2024

View reviewed changes

kamilmielnik requested a review from a team February 7, 2024 07:51

kulyk added 5 commits February 7, 2024 09:25

Merge branch 'master' into virtual-dashcard-types

ef22e7f

Fix isQuestionDashCard type guard

0f8540c

Avoid type-casting for getVisualizationRaw

61c17a7

Use type guard for dashcard series attr

5e7d3eb

Avoid type casting in parameter mapping utils

bcd3dc2

kulyk requested a review from kamilmielnik February 7, 2024 09:56

Fix type casting

1ed9a9a

kamilmielnik approved these changes Feb 7, 2024

View reviewed changes

kamilmielnik requested a review from a team February 7, 2024 10:27

kulyk merged commit 12db70d into master Feb 7, 2024
109 checks passed

kulyk deleted the virtual-dashcard-types branch February 7, 2024 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up dashboard card types #38444

Clean up dashboard card types #38444

kulyk commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

replay-io bot commented Feb 5, 2024 •

edited

Loading

kamilmielnik left a comment

kamilmielnik Feb 6, 2024

kulyk Feb 7, 2024

kamilmielnik Feb 6, 2024

kulyk Feb 7, 2024 •

edited

Loading

kamilmielnik Feb 7, 2024

kamilmielnik Feb 7, 2024

kulyk Feb 7, 2024

kamilmielnik Feb 7, 2024

kulyk Feb 7, 2024

kamilmielnik Feb 7, 2024

kulyk Feb 7, 2024

kamilmielnik Feb 7, 2024

kulyk Feb 7, 2024

kulyk commented Feb 7, 2024

kamilmielnik left a comment

github-actions bot commented Feb 7, 2024

		): dashcard is QuestionDashboardCard {
		return "card_id" in dashcard && "card" in dashcard;

	if ("series" in dashcard && Array.isArray(dashcard.series)) {
	if (isQuestionDashCard(dashcard)) {

Clean up dashboard card types #38444

Clean up dashboard card types #38444

Conversation

kulyk commented Feb 5, 2024 • edited Loading

github-actions bot commented Feb 5, 2024 • edited Loading

replay-io bot commented Feb 5, 2024 • edited Loading

kamilmielnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kulyk Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kulyk commented Feb 7, 2024

kamilmielnik left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2024

kulyk commented Feb 5, 2024 •

edited

Loading

github-actions bot commented Feb 5, 2024 •

edited

Loading

replay-io bot commented Feb 5, 2024 •

edited

Loading

kulyk Feb 7, 2024 •

edited

Loading