feat: DAG Behavioral Changes (Canvas resource, Source Models, ... buttons)#8715
feat: DAG Behavioral Changes (Canvas resource, Source Models, ... buttons)#8715royendo wants to merge 150 commits into
Conversation
royendo
left a comment
There was a problem hiding this comment.
Code Review - DAG Nit Behavioral Fixes
Good UX improvements with the dropdown menu replacing the old toolbar. Here's my feedback:
Issues to Address
1. ResourceNode.svelte - Code duplication
The node markup is duplicated for error vs non-error cases (lines ~134-230 and ~240-330). This makes maintenance harder. Consider extracting the common parts:
<!-- Common node wrapper -->
{#snippet nodeContent()}
<Handle ... />
<Handle ... />
{#if !isInOverlay}
<div class="node-menu">...</div>
{/if}
<div class="icon-wrapper">...</div>
<div class="details">...</div>
{/snippet}
{#if hasError}
<Tooltip>
<div class="node" ...>
{@render nodeContent()}
</div>
<TooltipContent>...</TooltipContent>
</Tooltip>
{:else}
<div class="node" ...>
{@render nodeContent()}
</div>
{/if}2. ResourceNode.svelte - Import formatting (line ~17)
import { GitFork } from "lucide-svelte";
import { builderActions, getAttrs } from "bits-ui";The GitFork import has inconsistent indentation.
3. ResourceNode.svelte - Unused import
NodeToolbar is still in the imports but was removed from usage. Clean this up.
4. ResourceNode.svelte - handleRefresh doesn't show feedback
function handleRefresh(e?: MouseEvent) {
e?.stopPropagation();
if (!isModel || !data?.resource?.meta?.name?.name) return;
void $triggerMutation.mutateAsync({...});
}Consider adding loading state or toast notification so users know the refresh was triggered.
5. GraphCanvas.svelte - Global CSS selector
:global(.svelte-flow .svelte-flow__pane) {
background-color: var(--surface-background, #ffffff);
}This could unintentionally affect other xyflow instances in the app. Consider scoping it more specifically or using a data attribute.
Minor Suggestions
- The icon size changed from
20pxto16pxin the node - was this intentional? - Consider memoizing
handleViewGraphsince it accesses reactive stores
Otherwise the dropdown approach is much cleaner than the old toolbar!
Error case wraps in with error tooltip content
|
|
A few follow up items discussed with Di once this is in
collapse all filters into a dropdown like linear |
# Conflicts: # web-common/src/layout/ApplicationHeader.svelte
Di7design
left a comment
There was a problem hiding this comment.
The functionality is approved. The filter bar will be in another PR, and the information box needs more design work later.
Brings in 46 commits from main, most notably: - #8912 Cloud editing MVP: branch deployment selector pill, `@branch` path-segment routing, "Open in editor" affordance. - #9296 Table toolbar simplifications (bindable API, new `onFilterChange(key, selected)` signature). - #9187 Chart type selector in explore (replaces `chart_force_line` proto field with `reserved 36`). Conflicts resolved: - `resource-graph/graph-canvas/ResourceNode.svelte`: kept this branch's refactored node structure (horizontal handles, inspect-store, test-failure marker, loading spinner), and ported main's "Open in editor" affordance — `NodeToolbar` with an `ExternalLink` button shown when a node is selected and resolves to a file artifact. Skipped main's error popover since this branch surfaces errors via the inspect store. Added `.toolbar-open-btn` styles. - `resource-graph/summary/SummaryGraph.svelte` and `summary/SummaryNode.svelte`: kept this branch's deletes (the summary surface was superseded). Main's `branchSearchSuffix` change to those files goes nowhere — the files no longer exist on this branch. - `web-local/.../graph/+page.svelte`: kept this branch's rich implementation. Main's replacement (`<GraphWorkspace />`) is a cloud-only entry point that doesn't fit `web-local`. - `models/navigation/ModelMenuItems.svelte`: combined imports — kept this branch's `resourceShorthandMapping` / `ResourceKind`, added main's `navigateToFile`, restored a missing `goto` import for `viewGraph`. Dropped the unused `openResourceGraphQuickView` import. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Removes the toolbar button that was ported in during the main merge. Drops the `NodeToolbar`, the `openFile` function, the `artifact` derivation, the imports for `fileArtifacts` / `navigateToFile` / `ExternalLink`, and the `.toolbar-open-btn` styles. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Connector resources stay in the partition adjacency (so the sidebar still groups resources by their connector and the OLAP "show all" entry still works) but no longer render as nodes on the canvas. This matches the existing dashboard-tree partition's behavior, which already excluded connector nodes and surfaced their info as badges on Source/Model nodes. Splits the rendering allowlist out of `ALLOWED_KINDS` into a new `RENDERABLE_KINDS` set used by `buildResourceGraph`. Drops the now dead `Connector` switch arm and the `kind !== Connector` exception on the hidden-resource skip. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The runtime parser leaves `Kind` blank on some refs (e.g., a metrics view's `model:` field) so the runtime can later infer whether the target is a Source, Model, or external table. Until that inference completes — or in cases where it never resolves cleanly — those refs reach the frontend with `kind === undefined`. The graph then can't stringify them to a `kind:name` lookup key, so the edge from MetricsView → Model is dropped, the model is filtered out as "isolated", and the chain only appears once a dashboard adds a kind-bearing ref upstream. Add a `buildNameIndex` / `resolveRefId` pair in `graph-builder.ts` that falls back to a name-only lookup when the name is unambiguous within the visible resource set. Use them in: - `buildResourceGraph` ref edges (renders the model→MV edge on canvas). - `partitionResourcesByMetrics` adjacency (groups MV with its model). - `buildDirectedAdjacency` (used by the dashboard-tree partition). - `dashboardConnectedIds` BFS in `ResourceGraph.svelte` (so the model isn't filtered out as "isolated" when it's actually upstream of a MV). Ambiguous names (multiple resources share the name) intentionally don't resolve — kind is required to disambiguate. Tests still pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CI svelte-check caught two more menu-item files that call `goto` for the "view in graph" navigation but never imported it. Same pattern as the earlier fix to `ModelMenuItems.svelte`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| } | ||
|
|
||
| // Track canvas | ||
| // Collect MetricsView refs from inline components for direct Canvas -> MetricsView links in the DAG. |
There was a problem hiding this comment.
I don't think this flattening of the DAG into the canvas' refs inside the parser is justified just for UI convenience. It has negative implications various other places, such as how the DAG is reported to AI and how reconciles get scheduled.
If needed, I suggest resolving this on the fly inside the DAG UI code instead.
There was a problem hiding this comment.
hm, i wonder the performance implications of this (will poke around), i was actually going to pull out the backend code out of this PR (to bring it lines changed down) but sounds like we dont want to do this at all?
There was a problem hiding this comment.
Oh Linear isnt synced weird:
hm, i wonder the performance implications of this (will poke around), i was actually going to pull out the backend code out of this PR (to bring it lines changed down) but sounds like we dont want to do this at all?
| } else { | ||
| res.Kind, err = ParseResourceKind(v) |
There was a problem hiding this comment.
The remaining backend changes are no-ops (same behavior as before) as far as I can tell, so would prefer no diff at all then on the backend.
https://www.loom.com/share/69c90f1c41df4781bfae51fb63b1966a
Checklist: