feat(ui): support explicit dagster/ui_color tags for asset backgrounds#33670
feat(ui): support explicit dagster/ui_color tags for asset backgrounds#33670vidiyala99 wants to merge 2 commits intodagster-io:masterfrom
Conversation
Greptile SummaryThis PR adds support for a new The implementation is clean and type-safe, correctly using existing Dagster Gaps and suggestions:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| js_modules/ui-core/src/asset-graph/AssetNode.tsx | Adds getCustomBackgroundForAsset helper and $customBackground styled-component prop to AssetNodeBox to support dagster/ui_color tag-driven background colors. The feature is well-scoped and uses existing Colors tokens correctly, but has coverage gaps: the minimal (zoomed-out) views are not updated, the name-header row keeps its own background, and two tag key variants are accepted without documentation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AssetNodeFragment.tags] --> B{Find dagster/ui_color\nor dagster/ui/color tag}
B -- Not found --> C[return undefined]
B -- Found --> D{tag value\nlowercase}
D -- red --> E1[Colors.backgroundRed]
D -- yellow --> E2[Colors.backgroundYellow]
D -- green --> E3[Colors.backgroundGreen]
D -- blue --> E4[Colors.backgroundBlue]
D -- olive --> E5[Colors.backgroundOlive]
D -- cyan --> E6[Colors.backgroundCyan]
D -- lime --> E7[Colors.backgroundLime]
D -- gray --> E8[Colors.backgroundGray]
D -- other --> C
E1 & E2 & E3 & E4 & E5 & E6 & E7 & E8 --> F[$customBackground prop]
C --> G[Colors.backgroundDefault fallback]
F --> H[AssetNodeBox background CSS]
G --> H
H -.->|NOT applied| I[AssetNodeMinimalWithHealth\nAssetNodeMinimalWithoutHealth\nAssetName header]
Comments Outside Diff (1)
-
js_modules/ui-core/src/asset-graph/AssetNode.tsx, line 586-609 (link)Custom background not applied to minimal (zoomed-out) views
The
dagster/ui_colortag is only respected in the fullAssetNodeBoxview. TheAssetNodeMinimalWithHealthandAssetNodeMinimalWithoutHealthcomponents (used when the DAG is zoomed out) still derive their background from health/status data only. A user who sets adagster/ui_colortag will see the custom color only when zoomed in, creating an inconsistent experience.If this is intentional (e.g., to avoid conflicting with status-based coloring in the minimal view), it's worth noting this limitation in a comment or in the PR description so future contributors know it's a deliberate choice.
Consider applying
getCustomBackgroundForAssetin both minimal-view components when present, or at least document why it's excluded.
Reviews (1): Last reviewed commit: "feat(ui): support explicit dagster/ui_co..." | Re-trigger Greptile
| tags?: {key: string; value: string}[], | ||
| ): string | undefined => { | ||
| const uiColorTag = tags?.find( | ||
| (t) => t.key === 'dagster/ui_color' || t.key === 'dagster/ui/color', |
There was a problem hiding this comment.
Accepting two tag key formats without documentation
The function silently accepts both dagster/ui_color (underscore) and dagster/ui/color (slash). These are semantically distinct tag keys — dagster/ui_color is in the dagster namespace with key ui_color, while dagster/ui/color would be interpreted as being under the dagster/ui namespace. The PR description only mentions dagster/ui_color.
Supporting both quietly means users may unknowingly set either form and have it work, leading to inconsistent tag usage across codebases. If dagster/ui/color is intentional (e.g., as an alias for backward compat), consider documenting this in a comment.
| tags?: {key: string; value: string}[], | |
| ): string | undefined => { | |
| const uiColorTag = tags?.find( | |
| (t) => t.key === 'dagster/ui_color' || t.key === 'dagster/ui/color', | |
| const uiColorTag = tags?.find( | |
| (t) => t.key === 'dagster/ui_color', | |
| ); |
Or if the alias is intentional, add a clarifying comment:
| tags?: {key: string; value: string}[], | |
| ): string | undefined => { | |
| const uiColorTag = tags?.find( | |
| (t) => t.key === 'dagster/ui_color' || t.key === 'dagster/ui/color', | |
| // Accept both 'dagster/ui_color' and 'dagster/ui/color' as aliases | |
| const uiColorTag = tags?.find( | |
| (t) => t.key === 'dagster/ui_color' || t.key === 'dagster/ui/color', | |
| ); |
| }`}; | ||
| ${(p) => p.$selected && `outline: 2px solid ${Colors.lineageNodeBorderSelected()}`}; | ||
|
|
||
| background: ${Colors.backgroundDefault()}; | ||
| background: ${(p) => p.$customBackground || Colors.backgroundDefault()}; |
There was a problem hiding this comment.
Custom background visible only in body rows, not the name header
AssetNodeBox is the outer container, but AssetName (the header that displays the asset name) has its own hardcoded background property — Colors.lineageNodeBackground() or Colors.backgroundLight(). As a result, the $customBackground is only visible in the content rows below the header, while the name header always remains the default color. This may produce a visually inconsistent appearance (e.g., a red body with a default-color header).
If the intent is to tint the entire node, the AssetName background should also be updated. If tinting only the body is intentional, a comment here explaining the behavior would be helpful.
Summary
Test Plan