feat: fixes routing rules tree view with better layout and node UI#2407
Merged
Pratham-Mishra04 merged 1 commit intov1.5.0from Apr 2, 2026
Conversation
18 tasks
|
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughSummary by CodeRabbit
WalkthroughExtracts routing-tree logic into dedicated frontend modules (CEL parsing, trie/DAG graph construction, layout, React Flow nodes/edges, position persistence), refactors routingTreeView to compose them with selection/highlighting and layout persistence, and changes a backend JSON tag so Changes
Sequence DiagramsequenceDiagram
participant UI as routingTreeView (Client)
participant Parser as CEL Parser
participant Trie as Trie Builder
participant Merger as Subtree Merger
participant DAG as DAG Collector
participant Layout as Dagre Layout
participant RF as React Flow Renderer
UI->>Parser: expandCEL / normalizeCond for rules
Parser->>Trie: insert normalized condition paths
Trie->>Trie: attach deduplicated rule terminals
Trie->>Merger: provide root trie
Merger->>DAG: produce layered DAG structure
DAG->>Parser: evalChainCondition for chain rules
DAG->>Layout: send nodes+edges (exclude chain-back edges)
Layout->>RF: positioned nodes & edges
DAG->>RF: add chain-back edges (weak/strong) and metadata
RF->>UI: render RFSource / RFCondition / RFRule nodes + RfChainEdge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
9 tasks
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
roroghost17
force-pushed
the
03-30-feat_fixes_routing_rules_tree_view_with_better_layout_and_node_ui
branch
from
598f05b to
e6396bd
Compare
roroghost17
force-pushed
the
03-26-feat_adds_routing_tree_view
branch
from
6081883 to
c881ec9
Compare
Contributor
Confidence Score: 5/5
Important Files Changed
Reviews (6): Last reviewed commit: "feat: fixes routing rules tree view with..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (11)
ui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsx (1)
11-15: Consider adding type safety for node data.The
data: anytype loses type safety. Since the graph builder controls the data shape, consider defining an interface:♻️ Optional: Add typed data interface
+interface ConditionNodeData { + condition: string; + color?: string | null; + scopes?: string[]; +} + -export function RFConditionNode({ data }: { data: any }) { - const condition = data.condition as string; - const color = data.color as string | null; - const scopes = (data.scopes as string[] | undefined) ?? []; +export function RFConditionNode({ data }: { data: ConditionNodeData }) { + const { condition, color = null, scopes = [] } = data; const accent = color ?? undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsx` around lines 11 - 15, The component RFConditionNode currently types props as data: any, losing type safety; define a specific interface (e.g., RFConditionNodeData) that describes condition: string, color?: string | null, scopes?: string[] and use it in the component signature (RFConditionNode({ data }: { data: RFConditionNodeData }) or a typed prop) and update usages of condition, color, scopes, accent to rely on that interface instead of casting—this will ensure compile-time checks and safer refactors for RFConditionNode and any callers that construct its data.ui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsx (1)
7-7: Exported constant not used in component styles.
RF_HANDLE_SIZE_PXis exported (presumably for external calculations) but the actual sizing in line 19 uses hardcoded14pxvalues. Consider using a CSS custom property or inline style to keep them in sync:♻️ Optional: Use the constant for sizing
export function RFEdgeHandle({ className, accentColor, style, ...rest }: RFEdgeHandleProps) { return ( <Handle className={cn( "!z-0 !pointer-events-auto", - "!h-[14px] !w-[14px] !min-h-[14px] !min-w-[14px]", + "!rounded-full !border-0 !border-none !p-0 !shadow-none", "!rounded-full !border-0 !border-none !p-0 !shadow-none", className, )} - style={{ - ...style, - ...(accentColor ? { background: accentColor } : {}), - }} + style={{ + width: RF_HANDLE_SIZE_PX, + height: RF_HANDLE_SIZE_PX, + minWidth: RF_HANDLE_SIZE_PX, + minHeight: RF_HANDLE_SIZE_PX, + ...style, + ...(accentColor ? { background: accentColor } : {}), + }} {...rest} /> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsx` at line 7, The exported RF_HANDLE_SIZE_PX constant is not used in the component styles; replace the hardcoded "14px" values inside the RFEdgeHandle component (or related style block in rfEdgeHandle.tsx) with a single source of truth by either setting a CSS custom property (e.g. --rf-handle-size) to `${RF_HANDLE_SIZE_PX}px` on the element or by applying an inline style that uses RF_HANDLE_SIZE_PX to compute width/height/border-radius, so sizing stays in sync with the exported constant.ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx (4)
61-62: Consider using the ReactFlowInstance type.The
anytype with eslint-disable could be replaced with the proper type from@xyflow/react.♻️ Suggested type improvement
+import type { ReactFlowInstance } from "@xyflow/react"; ... - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const rfInstanceRef = useRef<any>(null); + const rfInstanceRef = useRef<ReactFlowInstance | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx` around lines 61 - 62, Replace the any-typed ref by importing and using the proper ReactFlowInstance type from '@xyflow/react' and remove the eslint-disable; specifically change the rfInstanceRef declaration (currently const rfInstanceRef = useRef<any>(null)) to useRef<ReactFlowInstance | null>(null) and add the corresponding import for ReactFlowInstance so the ref is strongly typed throughout the component.
399-405: Nested ternary reduces readability.The nested ternary for displaying rule count could be extracted for clarity.
♻️ Suggested extraction
+ const ruleCountText = useMemo(() => { + if (!search) return `${rules.length} rule${rules.length !== 1 ? "s" : ""}`; + if (highlightedIds && highlightedIds.size > 0) { + return `${matchCount} rule${matchCount !== 1 ? "s" : ""}`; + } + return "no match"; + }, [search, highlightedIds, matchCount, rules.length]); ... <p className="text-[11px] text-muted-foreground"> - {search - ? highlightedIds && highlightedIds.size > 0 - ? `${matchCount} rule${matchCount !== 1 ? "s" : ""}` - : "no match" - : `${rules.length} rule${rules.length !== 1 ? "s" : ""}`} + {ruleCountText} </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx` around lines 399 - 405, The nested ternary in the JSX reduces readability; extract the logic that builds the display string into a small helper/variable (e.g., computeRuleCountText or ruleCountText) inside the RoutingTreeView component and return a single string to render. Implement the helper to accept/close over search, highlightedIds, matchCount and rules.length and produce the three possible outputs ("no match", "{matchCount} rule(s)", or "{rules.length} rule(s)") with proper pluralization, then replace the nested ternary in the <p> with that single variable or function call.
180-186: Chain edge identification is coupled to ID naming convention.The chain-back edge exclusion relies on
e.id.endsWith("-chain"), which creates a tight coupling between this component and the edge ID format ingraphBuilder.ts. Consider using a data property instead for more explicit identification.♻️ Suggested approach
In
graphBuilder.ts, the edge already hasisChainBackduring construction. You could add adata.isChainBackproperty to the final edge:// In graphBuilder.ts rfEdges mapping data: { chainWeak: weak, isChainBack: true },Then in
routingTreeView.tsx:- if (!e.id.endsWith("-chain")) { + if (!(e.data as { isChainBack?: boolean })?.isChainBack) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx` around lines 180 - 186, The code in routingTreeView.tsx currently treats chain-back edges by checking e.id.endsWith("-chain"), which couples this view to the edge ID format; update the graph construction to set a canonical flag (e.g., add data.isChainBack = true in the rfEdges mapping inside graphBuilder.ts), then change the exclusion logic in routingTreeView.tsx to check that flag (e.g., if (!e.data?.isChainBack) { ... }) when populating parentsOf/childrenOf; update any references to e.id-based logic (the current e.id.endsWith("-chain") check) to use e.data.isChainBack so identification is explicit and decoupled from naming.
147-155: Verify timeout is necessary for fitView.The 50ms
setTimeoutdelay before callingfitViewsuggests a potential race condition with React state updates. This works but is fragile. Consider usingrequestAnimationFrameor ensuring the state flush completes before fitting.♻️ Alternative approach
- setTimeout(() => rfInstanceRef.current?.fitView({ padding: FIT_VIEW_PADDING, duration: 300 }), 50); + requestAnimationFrame(() => { + rfInstanceRef.current?.fitView({ padding: FIT_VIEW_PADDING, duration: 300 }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx` around lines 147 - 155, The setTimeout used in handleResetLayout to call rfInstanceRef.current?.fitView is fragile; replace it with a reliable post-render callback such as requestAnimationFrame (or ReactDOM.flushSync + immediate call) so fitView runs after the state updates complete. Update handleResetLayout to clear cookies and update cookieDataRef/current state (cookieDataRef, setNodes, setSelectedNodeId, setSelectedEdgeId) as before, then call rfInstanceRef.current?.fitView from inside requestAnimationFrame (or from an effect that watches baseNodes/nodes) to ensure layout fitting happens after React has flushed the DOM; reference handleResetLayout, rfInstanceRef, cookieDataRef, setNodes, setSelectedNodeId, and setSelectedEdgeId when locating the change.ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx (2)
13-18: Consider adding type safety for the node data prop.The
data: anytype loses type safety. Consider defining an interface for the node data structure.♻️ Suggested type definition
+interface RFRuleNodeData { + rule: RoutingRule; + scopeColor: string; +} + -export function RFRuleNode({ data }: { data: any }) { - const rule = data.rule as RoutingRule; - const scopeColor = data.scopeColor as string; +export function RFRuleNode({ data }: { data: RFRuleNodeData }) { + const { rule, scopeColor } = data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx` around lines 13 - 18, The RFRuleNode component currently types its props as any which loses type safety; define a specific interface (e.g., RFRuleNodeData or RFRuleNodeProps) describing the shape returned in data — include fields like rule: RoutingRule, scopeColor: string, and any other used members (e.g., rule.scope as ScopeKey and rule.targets: array) — then change the component signature to use that interface (export function RFRuleNode({ data }: RFRuleNodeProps) or similar) so usages of SCOPE_CONFIG, RoutingRule, ScopeKey, data.rule, data.scopeColor and rule.targets are type-checked.
80-133: Hover popover may cause layout shifts or overflow issues.The popover is positioned with
left-full ml-3, which could cause horizontal overflow when the node is near the right edge of the viewport. Consider adding boundary detection or using a tooltip library that handles positioning automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx` around lines 80 - 133, The popover anchored in the hovered block (when hovered is true) uses fixed classes left-full and ml-3 which can overflow the viewport; update the component to detect available space and flip or clamp the popover instead of always using left-full: add a small local state (e.g., popoverSide or popoverOffset) and compute placement in an effect using the node element's getBoundingClientRect (or swap in a positioning library like Popper.js), then conditionally apply classes (replace left-full ml-3 with right/full or a max-width & translate-x clamp) while still using rule, scopeColor and RenderProviderIcon as is so the visual content remains identical.ui/app/workspace/routing-rules/tree/views/graphBuilder.ts (2)
55-56: Consider adding proper types for LNode and LEdge.The
data: anyinLNodeand the unstructuredLEdgeinterface reduce type safety. Since these are internal types, consider defining stricter types for better maintainability.♻️ Suggested type definitions
-interface LNode { id: string; kind: "source" | "condition" | "rule" | "target"; data: any; w: number; h: number; } -interface LEdge { source: string; target: string; label?: string; color?: string; isChainBack?: boolean; isChainWeak?: boolean; sourceHandle?: string; targetHandle?: string; } +interface LNodeData { + condition?: string; + color?: string | null; + scopes?: string[]; + rule?: RoutingRule; + scopeColor?: string; +} + +interface LNode { + id: string; + kind: "source" | "condition" | "rule" | "target"; + data: LNodeData; + w: number; + h: number; +} + +interface LEdge { + source: string; + target: string; + label?: string; + color?: string; + isChainBack?: boolean; + isChainWeak?: boolean; + sourceHandle?: string; + targetHandle?: string; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts` around lines 55 - 56, The LNode and LEdge types are too loose; replace data: any in LNode with a discriminated union keyed by kind (e.g., SourceNodeData, ConditionNodeData, RuleNodeData, TargetNodeData) so each node kind exposes only the fields it needs, and tighten LEdge by explicitly typing optional fields (label: string, color: string, isChainBack: boolean, isChainWeak: boolean, sourceHandle?: string, targetHandle?: string) and any metadata shape used in edges; update any code referencing LNode or LEdge (e.g., graphBuilder functions that create/inspect nodes and edges) to use the new union members and adjust pattern matches/field access accordingly to satisfy the stricter types.
91-95: Duplicate rule check has O(n) complexity per insertion.The
findcall on line 94 scans the terminals array for each insertion. For rules with many paths, this could be slow. Consider using a Set for O(1) lookup.♻️ Suggested optimization
export interface TrieNode { id: string; condition: string | null; children: Map<string, TrieNode>; terminals: RoutingRule[]; + terminalIds: Set<string>; } ... const mkNode = (c: string | null): TrieNode => - ({ id: c === null ? "root" : `n${++uid}`, condition: c, children: new Map(), terminals: [] }); + ({ id: c === null ? "root" : `n${++uid}`, condition: c, children: new Map(), terminals: [], terminalIds: new Set() }); ... - if (!node.terminals.find((r) => r.id === rule.id)) node.terminals.push(rule); + if (!node.terminalIds.has(rule.id)) { + node.terminalIds.add(rule.id); + node.terminals.push(rule); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts` around lines 91 - 95, The current insertion into the trie uses node.terminals.find((r) => r.id === rule.id) which is O(n) per insert; change node.terminals to maintain an O(1) lookup (e.g., add a node.terminalIds: Set<string> alongside node.terminals array or replace terminals with a Map/Set) and update the insertion logic in the graph builder (the block using mkNode, node.children, node.terminals, and rule.id) to check terminalIds.has(rule.id) before pushing to node.terminals and to add rule.id to terminalIds when inserting so duplicate checks become O(1).ui/components/ui/asyncMultiselect.tsx (1)
459-459: Consider making the 160px menu height opt-in instead of the shared default.
AsyncMultiSelectis a base UI component, so this changes every caller, not just the new routing-rules tree. If the tighter menu is only needed for that flow, threadminMenuHeightthrough props and set160at the callsite instead of hard-coding it here.As per coding guidelines,
**: always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/asyncMultiselect.tsx` at line 459, The hard-coded minMenuHeight=160 in AsyncMultiSelect should be made configurable: add an optional prop (e.g., minMenuHeight?: number) to the AsyncMultiSelect props/interface and replace the literal 160 with the prop value (use the prop when provided, otherwise fall back to the component's existing default behavior or undefined), then update the routing-rules tree callsite to pass minMenuHeight={160} where the tighter menu is required; reference AsyncMultiSelect and the minMenuHeight prop in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts`:
- Around line 55-56: The LNode and LEdge types are too loose; replace data: any
in LNode with a discriminated union keyed by kind (e.g., SourceNodeData,
ConditionNodeData, RuleNodeData, TargetNodeData) so each node kind exposes only
the fields it needs, and tighten LEdge by explicitly typing optional fields
(label: string, color: string, isChainBack: boolean, isChainWeak: boolean,
sourceHandle?: string, targetHandle?: string) and any metadata shape used in
edges; update any code referencing LNode or LEdge (e.g., graphBuilder functions
that create/inspect nodes and edges) to use the new union members and adjust
pattern matches/field access accordingly to satisfy the stricter types.
- Around line 91-95: The current insertion into the trie uses
node.terminals.find((r) => r.id === rule.id) which is O(n) per insert; change
node.terminals to maintain an O(1) lookup (e.g., add a node.terminalIds:
Set<string> alongside node.terminals array or replace terminals with a Map/Set)
and update the insertion logic in the graph builder (the block using mkNode,
node.children, node.terminals, and rule.id) to check terminalIds.has(rule.id)
before pushing to node.terminals and to add rule.id to terminalIds when
inserting so duplicate checks become O(1).
In `@ui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsx`:
- Around line 11-15: The component RFConditionNode currently types props as
data: any, losing type safety; define a specific interface (e.g.,
RFConditionNodeData) that describes condition: string, color?: string | null,
scopes?: string[] and use it in the component signature (RFConditionNode({ data
}: { data: RFConditionNodeData }) or a typed prop) and update usages of
condition, color, scopes, accent to rely on that interface instead of
casting—this will ensure compile-time checks and safer refactors for
RFConditionNode and any callers that construct its data.
In `@ui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsx`:
- Line 7: The exported RF_HANDLE_SIZE_PX constant is not used in the component
styles; replace the hardcoded "14px" values inside the RFEdgeHandle component
(or related style block in rfEdgeHandle.tsx) with a single source of truth by
either setting a CSS custom property (e.g. --rf-handle-size) to
`${RF_HANDLE_SIZE_PX}px` on the element or by applying an inline style that uses
RF_HANDLE_SIZE_PX to compute width/height/border-radius, so sizing stays in sync
with the exported constant.
In `@ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx`:
- Around line 13-18: The RFRuleNode component currently types its props as any
which loses type safety; define a specific interface (e.g., RFRuleNodeData or
RFRuleNodeProps) describing the shape returned in data — include fields like
rule: RoutingRule, scopeColor: string, and any other used members (e.g.,
rule.scope as ScopeKey and rule.targets: array) — then change the component
signature to use that interface (export function RFRuleNode({ data }:
RFRuleNodeProps) or similar) so usages of SCOPE_CONFIG, RoutingRule, ScopeKey,
data.rule, data.scopeColor and rule.targets are type-checked.
- Around line 80-133: The popover anchored in the hovered block (when hovered is
true) uses fixed classes left-full and ml-3 which can overflow the viewport;
update the component to detect available space and flip or clamp the popover
instead of always using left-full: add a small local state (e.g., popoverSide or
popoverOffset) and compute placement in an effect using the node element's
getBoundingClientRect (or swap in a positioning library like Popper.js), then
conditionally apply classes (replace left-full ml-3 with right/full or a
max-width & translate-x clamp) while still using rule, scopeColor and
RenderProviderIcon as is so the visual content remains identical.
In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx`:
- Around line 61-62: Replace the any-typed ref by importing and using the proper
ReactFlowInstance type from '@xyflow/react' and remove the eslint-disable;
specifically change the rfInstanceRef declaration (currently const rfInstanceRef
= useRef<any>(null)) to useRef<ReactFlowInstance | null>(null) and add the
corresponding import for ReactFlowInstance so the ref is strongly typed
throughout the component.
- Around line 399-405: The nested ternary in the JSX reduces readability;
extract the logic that builds the display string into a small helper/variable
(e.g., computeRuleCountText or ruleCountText) inside the RoutingTreeView
component and return a single string to render. Implement the helper to
accept/close over search, highlightedIds, matchCount and rules.length and
produce the three possible outputs ("no match", "{matchCount} rule(s)", or
"{rules.length} rule(s)") with proper pluralization, then replace the nested
ternary in the <p> with that single variable or function call.
- Around line 180-186: The code in routingTreeView.tsx currently treats
chain-back edges by checking e.id.endsWith("-chain"), which couples this view to
the edge ID format; update the graph construction to set a canonical flag (e.g.,
add data.isChainBack = true in the rfEdges mapping inside graphBuilder.ts), then
change the exclusion logic in routingTreeView.tsx to check that flag (e.g., if
(!e.data?.isChainBack) { ... }) when populating parentsOf/childrenOf; update any
references to e.id-based logic (the current e.id.endsWith("-chain") check) to
use e.data.isChainBack so identification is explicit and decoupled from naming.
- Around line 147-155: The setTimeout used in handleResetLayout to call
rfInstanceRef.current?.fitView is fragile; replace it with a reliable
post-render callback such as requestAnimationFrame (or ReactDOM.flushSync +
immediate call) so fitView runs after the state updates complete. Update
handleResetLayout to clear cookies and update cookieDataRef/current state
(cookieDataRef, setNodes, setSelectedNodeId, setSelectedEdgeId) as before, then
call rfInstanceRef.current?.fitView from inside requestAnimationFrame (or from
an effect that watches baseNodes/nodes) to ensure layout fitting happens after
React has flushed the DOM; reference handleResetLayout, rfInstanceRef,
cookieDataRef, setNodes, setSelectedNodeId, and setSelectedEdgeId when locating
the change.
In `@ui/components/ui/asyncMultiselect.tsx`:
- Line 459: The hard-coded minMenuHeight=160 in AsyncMultiSelect should be made
configurable: add an optional prop (e.g., minMenuHeight?: number) to the
AsyncMultiSelect props/interface and replace the literal 160 with the prop value
(use the prop when provided, otherwise fall back to the component's existing
default behavior or undefined), then update the routing-rules tree callsite to
pass minMenuHeight={160} where the tighter menu is required; reference
AsyncMultiSelect and the minMenuHeight prop in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22c92334-2509-4510-8856-7744c77d1200
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
framework/configstore/tables/routing_rules.goui/app/workspace/routing-rules/tree/views/celParser.tsui/app/workspace/routing-rules/tree/views/constants.tsui/app/workspace/routing-rules/tree/views/graphBuilder.tsui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsxui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsxui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsxui/app/workspace/routing-rules/tree/views/node/rfSourceNode.tsxui/app/workspace/routing-rules/tree/views/positionPersistence.tsui/app/workspace/routing-rules/tree/views/rfChainEdge.tsxui/app/workspace/routing-rules/tree/views/routingTreeView.tsxui/components/ui/asyncMultiselect.tsxui/package.json
roroghost17
force-pushed
the
03-30-feat_fixes_routing_rules_tree_view_with_better_layout_and_node_ui
branch
from
e6396bd to
242acaa
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
ui/app/workspace/routing-rules/tree/views/graphBuilder.ts (2)
400-401: Minor: Redundant chain-back filter.
computeLRLayoutalready filters out chain-back edges internally (line 332), so the filter on line 401 is redundant. Not harmful, but could be simplified.Remove redundant filter
- const positions = computeLRLayout(lNodes, lEdges.filter((e) => !e.isChainBack)); + const positions = computeLRLayout(lNodes, lEdges);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts` around lines 400 - 401, The call to computeLRLayout currently passes lEdges.filter((e) => !e.isChainBack) even though computeLRLayout already excludes chain-back edges internally; remove the redundant filter and pass lEdges directly (keep using lNodes and lEdges as the inputs), i.e., change the call that computes positions to call computeLRLayout(lNodes, lEdges) so the internal filtering in computeLRLayout handles chain-back edges.
293-300: Consider combining the fallback and iteration logic.When
entries.length === 0, you add an edge to source (line 296) and then iterate overentries(line 298) which will be a no-op. The logic is correct but could be clearer with anelseclause.Clarify control flow
const entries = findEntries(childrenOf.get("source") ?? [], vars); if (entries.length === 0) { // resolved vars match no condition node — fall back to source addEdge(ruleId, "source", "↺", sc, { isChainBack: true, isChainWeak: false, sourceHandle: "chain-out" }); + } else { + for (const { id: condId, strong } of entries) { + addEdge(ruleId, condId, "↺", sc, { isChainBack: true, isChainWeak: !strong, sourceHandle: "chain-out" }); + } } - for (const { id: condId, strong } of entries) { - addEdge(ruleId, condId, "↺", sc, { isChainBack: true, isChainWeak: !strong, sourceHandle: "chain-out" }); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts` around lines 293 - 300, The current code calls findEntries(childrenOf.get("source") ?? [], vars) into entries, then unconditionally checks if entries.length === 0 to addEdge(ruleId, "source", ...) and afterwards loops for (const { id: condId, strong } of entries) which is a no-op when empty; change this to an if/else so the fallback addEdge executes only when entries is empty and the for-loop executes only when entries has items—update the block that references findEntries, entries, addEdge, childrenOf, vars, ruleId, and sc to use an if (entries.length === 0) { addEdge(...) } else { for (...) { addEdge(...) } } structure to make control flow explicit.ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx (3)
61-64: Consider typing the React Flow instance ref.The
anytype suppression works but React Flow exportsReactFlowInstancetype that could be used for better type safety.Type the ref
+import type { ReactFlowInstance } from "@xyflow/react"; // ... - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const rfInstanceRef = useRef<any>(null); + const rfInstanceRef = useRef<ReactFlowInstance | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx` around lines 61 - 64, The rfInstanceRef is typed as any; replace useRef<any>(null) with useRef<ReactFlowInstance | null>(null) and import the ReactFlowInstance type from the reactflow package (e.g., import type { ReactFlowInstance } from 'reactflow'), remove the eslint-disable comment, and update any usages of rfInstanceRef.current to handle the nullable type (guard or optional chaining) so the code remains type-safe.
411-428: Consider addingdata-testidattributes to interactive toolbar elements.The search input and reset layout button lack
data-testidattributes. Per coding guidelines, interactive UI elements should have testids for e2e testing.Add testids
<Input value={search} onChange={(e) => setSearch(e.target.value)} placeholder="Search conditions or rules…" className="h-8 w-56 pl-8 text-sm" + data-testid="routing-tree-search-input" /> </div> <div className="h-5 w-px bg-border" /> <Button variant="ghost" size="sm" className="gap-1.5 text-muted-foreground hover:text-foreground" onClick={handleResetLayout} title="Reset to default layout" + data-testid="routing-tree-reset-btn" >As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern:data-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx` around lines 411 - 428, Add data-testid attributes to the interactive toolbar elements: add data-testid="routing-tree-search-input" to the Input used for search (the Input component bound to search with onChange) and add data-testid="routing-tree-reset-button" to the Button that calls handleResetLayout (the Button containing RotateCcw and "Reset layout"). Ensure attributes follow the pattern entity-element-qualifier and are added on the Input and Button elements in routingTreeView.tsx.
184-188: Chain-back edge detection relies on ID format convention.The detection
e.id.endsWith("-chain")depends on the edge ID format ingraphBuilder.ts(line 419:e-${le.source}-${le.target}${le.isChainBack ? "-chain" : ""}). This coupling is implicit.Consider documenting this convention or using the edge data to detect chain-back edges instead (the data includes
chainWeakfor chain edges).Use edge type instead of ID suffix
for (const e of edges) { if (!childrenOf.has(e.source)) childrenOf.set(e.source, []); childrenOf.get(e.source)!.push(e.target); // Exclude chain-back edges from the ancestor map (they reverse flow direction). - if (!e.id.endsWith("-chain")) { + if (e.type !== "rfChain") { if (!parentsOf.has(e.target)) parentsOf.set(e.target, []); parentsOf.get(e.target)!.push(e.source); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx` around lines 184 - 188, The current chain-back detection uses the edge id suffix check (e.id.endsWith("-chain")) which couples behavior to the ID format; instead, update the conditional to use the edge metadata (e.data?.chainWeak or the explicit chain flag available on the edge object) so chain-back edges are detected from their data rather than the id string: replace e.id.endsWith("-chain") with a check like e.data?.chainWeak === true (or the appropriate boolean flag on the edge) in the block that populates parentsOf (refer to the parentsOf map and the edge variable e), or alternately add a short comment documenting the ID convention if you intentionally keep the suffix approach.ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx (1)
13-18: Consider addingdata-testidfor the interactive rule node.This component renders an interactive, draggable node that users can hover and click. Per coding guidelines, interactive UI elements should have
data-testidattributes following the pattern<entity>-<element>-<qualifier>.Suggested testid addition
export function RFRuleNode({ data }: { data: any }) { const rule = data.rule as RoutingRule; const scopeColor = data.scopeColor as string; const cfg = SCOPE_CONFIG[rule.scope as ScopeKey]; - const multi = rule.targets.length > 1; + const multi = rule.targets?.length > 1; const [hovered, setHovered] = useState(false);And on the outer div:
return ( <div className="relative" style={{ width: RULE_W }} onMouseEnter={() => setHovered(true)} onMouseLeave={() => setHovered(false)} + data-testid={`routing-rule-node-${rule.id}`} >As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern:data-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx` around lines 13 - 18, Add a data-testid to the interactive outer element in RFRuleNode so tests can target it; specifically, on the outer div (the element that renders the draggable/hoverable node inside the RFRuleNode component) add a data-testid following the pattern "<entity>-<element>-<qualifier>" (e.g., "routing-rule-node-interactive" or use a unique instance-specific id like "routing-rule-node-<rule.id>") to make the element queryable in tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/components/ui/asyncMultiselect.tsx`:
- Line 459: The change reduces the AsyncMultiSelect prop minMenuHeight to 160
which globally affects all consumers (virtualKeySheet.tsx, valueEditor.tsx,
mcpServerSelector.tsx, mcpToolSelector.tsx, modelMultiselect.tsx,
entityAssociationSelect.tsx); confirm UX for long lists and either revert to the
previous default or make minMenuHeight configurable per consumer by adding a
prop (e.g., minMenuHeight) to the AsyncMultiSelect component and thread it
through where used so each consumer (notably mcpServerSelector.tsx,
mcpToolSelector.tsx, virtualKeySheet.tsx) can override the value to meet their
layout/scrolling needs. Ensure the AsyncMultiSelect default remains documented
and tests or screenshots from those six consumers are updated to validate the
visual impact.
---
Nitpick comments:
In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts`:
- Around line 400-401: The call to computeLRLayout currently passes
lEdges.filter((e) => !e.isChainBack) even though computeLRLayout already
excludes chain-back edges internally; remove the redundant filter and pass
lEdges directly (keep using lNodes and lEdges as the inputs), i.e., change the
call that computes positions to call computeLRLayout(lNodes, lEdges) so the
internal filtering in computeLRLayout handles chain-back edges.
- Around line 293-300: The current code calls
findEntries(childrenOf.get("source") ?? [], vars) into entries, then
unconditionally checks if entries.length === 0 to addEdge(ruleId, "source", ...)
and afterwards loops for (const { id: condId, strong } of entries) which is a
no-op when empty; change this to an if/else so the fallback addEdge executes
only when entries is empty and the for-loop executes only when entries has
items—update the block that references findEntries, entries, addEdge,
childrenOf, vars, ruleId, and sc to use an if (entries.length === 0) {
addEdge(...) } else { for (...) { addEdge(...) } } structure to make control
flow explicit.
In `@ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx`:
- Around line 13-18: Add a data-testid to the interactive outer element in
RFRuleNode so tests can target it; specifically, on the outer div (the element
that renders the draggable/hoverable node inside the RFRuleNode component) add a
data-testid following the pattern "<entity>-<element>-<qualifier>" (e.g.,
"routing-rule-node-interactive" or use a unique instance-specific id like
"routing-rule-node-<rule.id>") to make the element queryable in tests.
In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx`:
- Around line 61-64: The rfInstanceRef is typed as any; replace
useRef<any>(null) with useRef<ReactFlowInstance | null>(null) and import the
ReactFlowInstance type from the reactflow package (e.g., import type {
ReactFlowInstance } from 'reactflow'), remove the eslint-disable comment, and
update any usages of rfInstanceRef.current to handle the nullable type (guard or
optional chaining) so the code remains type-safe.
- Around line 411-428: Add data-testid attributes to the interactive toolbar
elements: add data-testid="routing-tree-search-input" to the Input used for
search (the Input component bound to search with onChange) and add
data-testid="routing-tree-reset-button" to the Button that calls
handleResetLayout (the Button containing RotateCcw and "Reset layout"). Ensure
attributes follow the pattern entity-element-qualifier and are added on the
Input and Button elements in routingTreeView.tsx.
- Around line 184-188: The current chain-back detection uses the edge id suffix
check (e.id.endsWith("-chain")) which couples behavior to the ID format;
instead, update the conditional to use the edge metadata (e.data?.chainWeak or
the explicit chain flag available on the edge object) so chain-back edges are
detected from their data rather than the id string: replace
e.id.endsWith("-chain") with a check like e.data?.chainWeak === true (or the
appropriate boolean flag on the edge) in the block that populates parentsOf
(refer to the parentsOf map and the edge variable e), or alternately add a short
comment documenting the ID convention if you intentionally keep the suffix
approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e749f71-1664-430e-8372-28b24e384a1d
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
framework/configstore/tables/routing_rules.goui/app/workspace/routing-rules/tree/views/celParser.tsui/app/workspace/routing-rules/tree/views/constants.tsui/app/workspace/routing-rules/tree/views/graphBuilder.tsui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsxui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsxui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsxui/app/workspace/routing-rules/tree/views/node/rfSourceNode.tsxui/app/workspace/routing-rules/tree/views/positionPersistence.tsui/app/workspace/routing-rules/tree/views/rfChainEdge.tsxui/app/workspace/routing-rules/tree/views/routingTreeView.tsxui/components/ui/asyncMultiselect.tsxui/package.json
✅ Files skipped from review due to trivial changes (6)
- ui/package.json
- framework/configstore/tables/routing_rules.go
- ui/app/workspace/routing-rules/tree/views/positionPersistence.ts
- ui/app/workspace/routing-rules/tree/views/node/rfSourceNode.tsx
- ui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsx
- ui/app/workspace/routing-rules/tree/views/constants.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsx
- ui/app/workspace/routing-rules/tree/views/rfChainEdge.tsx
- ui/app/workspace/routing-rules/tree/views/celParser.ts
roroghost17
force-pushed
the
03-30-feat_fixes_routing_rules_tree_view_with_better_layout_and_node_ui
branch
2 times, most recently
from
2f96f62 to
bff3dc0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/routing-rules/tree/views/celParser.ts`:
- Around line 79-115: The current expandCEL only fans out top-level ||, so
expressions like a && (b || c) remain as a single parenthesized part; update
expandCEL (and if helpful, splitOn/isWrappedInParens) to recursively distribute
ORs inside parenthesized groups: when splitting on "||" detect branches that are
parenthesized and themselves contain "||" (use isWrappedInParens to detect) then
call expandCEL on the inner content to produce multiple branch variants and
combine them with the surrounding AND parts (Cartesian product) so a && (b || c)
expands to ["a","b"] and ["a","c"]; add a regression unit test for the grouped
disjunction case (e.g., "a && (b || c)") to assert the fan-out occurs.
In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts`:
- Around line 140-163: collectTerminals() can return the same RoutingRule
multiple times (one per OR branch), which makes nodeColor() overweight scopes
for rules that expand into multiple terminals; change nodeColor() to deduplicate
rules by rule.id after calling collectTerminals() (e.g., build a Set of rule.id
or a Map from id→rule) and then compute counts only from the unique rules before
creating the counts Map, passing those deduped counts into blendColors and
caching the result; keep references to collectTerminals, nodeColor,
RoutingRule.rule.id, counts, SCOPE_CONFIG and blendColors to locate the changes.
- Around line 240-299: findEntries currently drops branches that evaluated to
null, causing the caller to treat unresolved fallbacks as static loops; update
findEntries (and its inner explore) to propagate "unknown" by pushing the
deepest condition node when a descendant returned null (emit the condition id
with strong:false), or return an explicit flag signaling unresolved; then update
the caller loop that consumes findEntries (the block that calls entries =
findEntries(...) and the fallback branch that calls addEdge(..., { isChainBack:
true, isChainWeak: false, ... })) to treat those propagated unresolved entries
as weak (set isChainWeak: true) instead of marking the chain as static.
In `@ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx`:
- Around line 21-26: The rule detail popover is only shown on mouse hover (the
div with onMouseEnter/onMouseLeave that sets setHovered and the similar handlers
in the 81-133 range), making it inaccessible to keyboard and touch users; update
these nodes to also open the same detail view on keyboard focus and activation
by adding onFocus/onBlur and keyboard handlers (onKeyDown for Enter/Space) and a
click toggle that calls the same setHovered/setDetailVisible state, and ensure
the rendered popover includes proper ARIA attributes (aria-haspopup,
aria-expanded) and focus management so it can be opened/closed via keyboard and
is reachable on touch devices.
In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx`:
- Around line 421-428: The new interactive controls in routingTreeView need
stable test selectors: add data-testid="routing-tree-reset-layout-btn" to the
Reset layout Button (the Button with onClick={handleResetLayout} and child
<RotateCcw />), and add data-testid="routing-tree-static-chain-info-trigger" and
data-testid="routing-tree-dynamic-chain-info-trigger" to the two legend info
trigger elements referenced around the legend rendering (the interactive
elements that open static/dynamic chain info); ensure attributes are added
directly on those interactive JSX elements so tests can target them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c0c9129-6882-4c98-8c24-bddc6e481854
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
framework/configstore/tables/routing_rules.goui/app/globals.cssui/app/workspace/routing-rules/tree/views/celParser.tsui/app/workspace/routing-rules/tree/views/constants.tsui/app/workspace/routing-rules/tree/views/graphBuilder.tsui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsxui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsxui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsxui/app/workspace/routing-rules/tree/views/node/rfSourceNode.tsxui/app/workspace/routing-rules/tree/views/positionPersistence.tsui/app/workspace/routing-rules/tree/views/rfChainEdge.tsxui/app/workspace/routing-rules/tree/views/routingTreeView.tsxui/components/ui/asyncMultiselect.tsxui/package.json
✅ Files skipped from review due to trivial changes (4)
- ui/package.json
- ui/app/globals.css
- ui/app/workspace/routing-rules/tree/views/positionPersistence.ts
- ui/app/workspace/routing-rules/tree/views/constants.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/configstore/tables/routing_rules.go
- ui/app/workspace/routing-rules/tree/views/node/rfSourceNode.tsx
- ui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsx
- ui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ui/app/workspace/routing-rules/tree/views/graphBuilder.ts (1)
148-164:⚠️ Potential issue | 🟡 MinorDeduplicate terminal rules before blending scope colors.
collectTerminals()returns the sameRoutingRuleonce per OR-expanded terminal path, sonodeColor()weights the blend by that raw array. A rule with more OR branches will skew the condition color toward its scope even though it's still only one rule. Consider counting uniquerule.ids before buildingcounts.🔧 Suggested fix
function nodeColor(node: TrieNode, cache?: Map<string, string | null>): string | null { if (cache?.has(node.id)) return cache.get(node.id)!; const rules = collectTerminals(node); if (!rules.length) { cache?.set(node.id, null); return null; } + // Deduplicate by rule ID to avoid OR-branch skew + const uniqueRules = [...new Map(rules.map((r) => [r.id, r])).values()]; // Count rules per scope to produce a weighted blend. const counts = new Map<string, number>(); - for (const r of rules) counts.set(r.scope, (counts.get(r.scope) ?? 0) + 1); + for (const r of uniqueRules) counts.set(r.scope, (counts.get(r.scope) ?? 0) + 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts` around lines 148 - 164, nodeColor currently counts each RoutingRule returned by collectTerminals including duplicates from OR-expanded paths, which skews blending; modify nodeColor (around the collectTerminals usage) to deduplicate rules by their unique rule.id before computing counts—e.g., build a Set or Map of rule.id to ensure each rule contributes only once, then iterate those unique rules to populate the counts Map and proceed with the existing color blending and cache behavior.
🧹 Nitpick comments (5)
ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx (2)
13-18: Consider adding a typed interface for thedataprop.Same as
RFConditionNode, usingdata: anyloses type safety. The expected shape is{ rule: RoutingRule; scopeColor: string }.♻️ Suggested type definition
+interface RFRuleNodeData { + rule: RoutingRule; + scopeColor: string; +} + -export function RFRuleNode({ data }: { data: any }) { - const rule = data.rule as RoutingRule; - const scopeColor = data.scopeColor as string; +export function RFRuleNode({ data }: { data: RFRuleNodeData }) { + const { rule, scopeColor } = data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx` around lines 13 - 18, The component RFRuleNode currently types its prop as any; define a specific props interface (e.g., RFRuleNodeProps) that matches the expected shape (at minimum rule: RoutingRule and scopeColor: string — include targets or other fields used by the component if present) and update the function signature to use RFRuleNodeProps instead of any; reference RFConditionNode as an example for the pattern and ensure imports/types for RoutingRule and ScopeKey remain used with SCOPE_CONFIG where needed.
81-133: Popover may overflow the viewport on rightmost nodes.The popover is absolutely positioned to the right of the node (
left-full ml-3). For rule nodes near the right edge of the canvas, this can push the popover outside the visible area. Consider detecting available space or using a library like RadixPopoverwith collision detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx` around lines 81 - 133, The popover rendered inside the hovered conditional (the JSX block that returns the div with class "absolute left-full ... ml-3" in rfRuleNode.tsx) can overflow on right-edge nodes; update this to use collision-aware placement (e.g., replace the manual absolutely positioned div with a Radix Popover or similar that supports collision detection and automatic flipping), or implement runtime placement logic: measure available space from the node (when hovered is true) and switch the popover container's placement class from "left-full ml-3" to a left-side anchor (e.g., right-full mr-3) when there isn’t enough room; ensure the change is applied where hovered is used and the popover wrapper div (the element currently using scopeColor for border) is replaced so targets mapping and styles remain unchanged.ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx (1)
60-64: Consider typing the React Flow instance ref.The
anytype with eslint-disable is understandable given React Flow's complex instance type, but you could useReactFlowInstancefrom@xyflow/reactfor better type safety.♻️ Suggested fix
+import type { ReactFlowInstance } from "@xyflow/react"; + // React Flow instance — captured via onInit so we can call fitView imperatively. -// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -const rfInstanceRef = useRef<any>(null); +const rfInstanceRef = useRef<ReactFlowInstance | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx` around lines 60 - 64, The rfInstanceRef currently uses an any type with an eslint-disable; change it to a properly typed React Flow instance by importing ReactFlowInstance from '@xyflow/react' and declaring rfInstanceRef as useRef<ReactFlowInstance | null>(null) (and remove the eslint-disable comment). Update references to rfInstanceRef elsewhere if needed to account for the nullable type (e.g., check for null before calling methods like fitView).ui/app/workspace/routing-rules/tree/views/graphBuilder.ts (1)
360-365: Consider extracting the source offset as a named constant.The hardcoded
200magic number for the source node's horizontal offset reduces readability. Consider adding it toconstants.tsalongside other layout values.♻️ Suggested refactor
In
constants.ts:export const SOURCE_LEFT_OFFSET = 200;In
graphBuilder.ts:+import { SOURCE_LEFT_OFFSET, ... } from "./constants"; // ... const sourcePos = positions.get("source"); -if (sourcePos) sourcePos.x -= 200; +if (sourcePos) sourcePos.x -= SOURCE_LEFT_OFFSET;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts` around lines 360 - 365, Replace the magic number used to offset the source node with a named constant: add SOURCE_LEFT_OFFSET to constants.ts and use it in graphBuilder.ts when adjusting the source position (the code around positions.get("source") and sourcePos.x -= 200); import SOURCE_LEFT_OFFSET and apply sourcePos.x -= SOURCE_LEFT_OFFSET to improve readability and centralize layout configuration.ui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsx (1)
11-14: Consider adding a typed interface for thedataprop.Using
data: anysacrifices type safety. A minimal interface would help catch mismatches betweengraphBuilder.tsand this component.♻️ Suggested type definition
+interface RFConditionNodeData { + condition: string; + color?: string | null; + scopes?: string[]; +} + -export function RFConditionNode({ data }: { data: any }) { - const condition = data.condition as string; - const color = data.color as string | null; - const scopes = (data.scopes as string[] | undefined) ?? []; +export function RFConditionNode({ data }: { data: RFConditionNodeData }) { + const condition = data.condition; + const color = data.color ?? null; + const scopes = data.scopes ?? [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsx` around lines 11 - 14, The component RFConditionNode currently types its prop as data: any; introduce a clear interface (e.g., RFConditionData or RFConditionNodeProps) that declares condition: string, color: string | null, and scopes?: string[] and use it in the function signature instead of any; update the destructuring to use that typed prop and export the interface so graphBuilder.ts can import/reuse it to keep both places in sync and catch mismatches early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts`:
- Around line 148-164: nodeColor currently counts each RoutingRule returned by
collectTerminals including duplicates from OR-expanded paths, which skews
blending; modify nodeColor (around the collectTerminals usage) to deduplicate
rules by their unique rule.id before computing counts—e.g., build a Set or Map
of rule.id to ensure each rule contributes only once, then iterate those unique
rules to populate the counts Map and proceed with the existing color blending
and cache behavior.
---
Nitpick comments:
In `@ui/app/workspace/routing-rules/tree/views/graphBuilder.ts`:
- Around line 360-365: Replace the magic number used to offset the source node
with a named constant: add SOURCE_LEFT_OFFSET to constants.ts and use it in
graphBuilder.ts when adjusting the source position (the code around
positions.get("source") and sourcePos.x -= 200); import SOURCE_LEFT_OFFSET and
apply sourcePos.x -= SOURCE_LEFT_OFFSET to improve readability and centralize
layout configuration.
In `@ui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsx`:
- Around line 11-14: The component RFConditionNode currently types its prop as
data: any; introduce a clear interface (e.g., RFConditionData or
RFConditionNodeProps) that declares condition: string, color: string | null, and
scopes?: string[] and use it in the function signature instead of any; update
the destructuring to use that typed prop and export the interface so
graphBuilder.ts can import/reuse it to keep both places in sync and catch
mismatches early.
In `@ui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsx`:
- Around line 13-18: The component RFRuleNode currently types its prop as any;
define a specific props interface (e.g., RFRuleNodeProps) that matches the
expected shape (at minimum rule: RoutingRule and scopeColor: string — include
targets or other fields used by the component if present) and update the
function signature to use RFRuleNodeProps instead of any; reference
RFConditionNode as an example for the pattern and ensure imports/types for
RoutingRule and ScopeKey remain used with SCOPE_CONFIG where needed.
- Around line 81-133: The popover rendered inside the hovered conditional (the
JSX block that returns the div with class "absolute left-full ... ml-3" in
rfRuleNode.tsx) can overflow on right-edge nodes; update this to use
collision-aware placement (e.g., replace the manual absolutely positioned div
with a Radix Popover or similar that supports collision detection and automatic
flipping), or implement runtime placement logic: measure available space from
the node (when hovered is true) and switch the popover container's placement
class from "left-full ml-3" to a left-side anchor (e.g., right-full mr-3) when
there isn’t enough room; ensure the change is applied where hovered is used and
the popover wrapper div (the element currently using scopeColor for border) is
replaced so targets mapping and styles remain unchanged.
In `@ui/app/workspace/routing-rules/tree/views/routingTreeView.tsx`:
- Around line 60-64: The rfInstanceRef currently uses an any type with an
eslint-disable; change it to a properly typed React Flow instance by importing
ReactFlowInstance from '@xyflow/react' and declaring rfInstanceRef as
useRef<ReactFlowInstance | null>(null) (and remove the eslint-disable comment).
Update references to rfInstanceRef elsewhere if needed to account for the
nullable type (e.g., check for null before calling methods like fitView).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdb11502-8c46-4214-a1e7-6143ee1a874d
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
framework/configstore/tables/routing_rules.goui/app/globals.cssui/app/workspace/routing-rules/tree/views/celParser.tsui/app/workspace/routing-rules/tree/views/constants.tsui/app/workspace/routing-rules/tree/views/graphBuilder.tsui/app/workspace/routing-rules/tree/views/node/rfConditionNode.tsxui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsxui/app/workspace/routing-rules/tree/views/node/rfRuleNode.tsxui/app/workspace/routing-rules/tree/views/node/rfSourceNode.tsxui/app/workspace/routing-rules/tree/views/positionPersistence.tsui/app/workspace/routing-rules/tree/views/rfChainEdge.tsxui/app/workspace/routing-rules/tree/views/routingTreeView.tsxui/components/ui/asyncMultiselect.tsxui/package.json
✅ Files skipped from review due to trivial changes (5)
- ui/package.json
- ui/app/workspace/routing-rules/tree/views/positionPersistence.ts
- ui/app/workspace/routing-rules/tree/views/node/rfEdgeHandle.tsx
- ui/app/workspace/routing-rules/tree/views/constants.ts
- ui/app/workspace/routing-rules/tree/views/celParser.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- framework/configstore/tables/routing_rules.go
- ui/app/workspace/routing-rules/tree/views/node/rfSourceNode.tsx
- ui/app/globals.css
Pratham-Mishra04
changed the base branch from
03-26-feat_adds_routing_tree_view
to
graphite-base/2407
roroghost17
force-pushed
the
graphite-base/2407
branch
from
c881ec9 to
52736a2
Compare
roroghost17
force-pushed
the
03-30-feat_fixes_routing_rules_tree_view_with_better_layout_and_node_ui
branch
from
bff3dc0 to
480d3b0
Compare
roroghost17
force-pushed
the
graphite-base/2407
branch
from
52736a2 to
be65bba
Compare
roroghost17
force-pushed
the
03-30-feat_fixes_routing_rules_tree_view_with_better_layout_and_node_ui
branch
from
480d3b0 to
e36f409
Compare
Pratham-Mishra04
previously approved these changes
Apr 2, 2026
Contributor
Merge activity
|
akshaydeo
dismissed
Pratham-Mishra04’s stale review
The base branch was changed.
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
Pratham-Mishra04
force-pushed
the
03-30-feat_fixes_routing_rules_tree_view_with_better_layout_and_node_ui
branch
from
e36f409 to
22f1a40
Compare
Pratham-Mishra04
approved these changes
Apr 2, 2026
Pratham-Mishra04
deleted the
03-30-feat_fixes_routing_rules_tree_view_with_better_layout_and_node_ui
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements a new routing rules tree visualization that displays routing rules as an interactive directed graph. The tree view shows the logical flow from incoming requests through conditions to rule execution, with support for chain rules that loop back into the evaluation process.
Changes
omitemptyfromTargetsfield inTableRoutingRuleto ensure targets are always serializedType of change
Affected areas
How to test
Navigate to the routing rules page and click the tree view tab to see the new visualization. Create routing rules with different scopes and CEL expressions to test the graph generation and layout.
Screenshots/Recordings
The new tree view provides an interactive graph showing:
Breaking changes
Related issues
Implements routing rules tree visualization feature request.
Security considerations
No security implications - this is a read-only visualization of existing routing rule data.
Checklist
docs/contributing/README.mdand followed the guidelines