-
Notifications
You must be signed in to change notification settings - Fork 164
feat: nit dag changes #8715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: nit dag changes #8715
Conversation
royendo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
idea is to change a bit of the behavior to prep for Rill Cloud Integration
https://www.loom.com/share/f21f8e222af24cc78f843d87d0611d0e
instead of Click opening blue Open
add dropdown to each node,
doesnt show for overlays.
adds canvas as a component
Checklist: