move some handlers to useAgentSelectionSync, remove useDefaultSubAgentNodeIdRef#2855
move some handlers to useAgentSelectionSync, remove useDefaultSubAgentNodeIdRef#2855dimaMachina merged 18 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 0cc9b74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Extracts selection-sync logic and related handlers ( Key changes
Summary ｜ 5 files ｜ 14 commits ｜ base: Selection sync extracted into
|
|
TL;DR — Extracts selection-sync logic (canvas selection ↔ URL query state) and related handlers from the monolithic agent page component into a dedicated Key changes
Summary ｜ 6 files ｜ 18 commits ｜ base: Selection-sync extracted into
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean extraction — the hook consolidation and useDefaultSubAgentNodeIdRef removal both look correct. One minor formatting issue below.
Claude Opus ｜ 𝕏
| }} | ||
| onBeforeDelete={async (state) => { | ||
| const defaultSubAgentNodeId = defaultSubAgentNodeIdRef.current; | ||
| const defaultSubAgentNodeId = form.getValues('defaultSubAgentNodeId') |
There was a problem hiding this comment.
Missing semicolon — Biome enforces "semicolons": "always".
| const defaultSubAgentNodeId = form.getValues('defaultSubAgentNodeId') | |
| const defaultSubAgentNodeId = form.getValues('defaultSubAgentNodeId'); |
There was a problem hiding this comment.
New commits fix the missing semicolon and biome.jsonc formatting. LGTM.
Claude Opus ｜ 𝕏
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
page.client.tsx:649Missing semicolon violates biome formatting rules
💭 Consider (1) 💭
💭 1) use-agent-selection-sync.ts:103-105 Memoize selectedNode/selectedEdge lookups
Issue: The selectedNode and selectedEdge values are computed on every render via findNodeByGraphKey and findEdgeByGraphKey, which perform O(n) array lookups.
Why: When nodes or edges arrays are large, this could add measurable overhead during rapid state updates (e.g., dragging nodes). However, the hook uses the 'use memo' directive, which may already handle this at the compiler level if React Compiler is enabled.
Fix: If React Compiler is not active, consider wrapping these in useMemo:
const selectedNode = useMemo(
() => findNodeByGraphKey(nodes, nodeId),
[nodes, nodeId]
);
const selectedEdge = useMemo(
() => findEdgeByGraphKey(edges, nodes, edgeId),
[edges, nodes, edgeId]
);Refs:
💡 APPROVE WITH SUGGESTIONS
Summary: Clean refactoring that consolidates selection-related logic into a well-organized custom hook. The removal of useDefaultSubAgentNodeIdRef in favor of direct form.getValues() calls is a simplification that maintains correct behavior. One minor formatting fix needed (missing semicolon). Nice work on improving code organization! 🎉
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
use-agent-selection-sync.ts:38-57 |
onChange callback captures isOpen in closure |
Intentional design — the defaultPane logic depends on current isOpen state. LOW confidence finding. |
use-animate-graph.ts:38 |
Pattern change from ref subscription to direct form read | Both reviewers agreed this is an improvement, not a regression. Reading at event time is correct. |
use-agent-selection-sync.ts:98-101 |
clearCanvasSelection creates new arrays via .map() | Only called on user actions, not on every render. Necessary for immutable state updates. |
use-agent-selection-sync.ts |
General hook structure | The hook is well-organized with clear single responsibility. |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
5 | 0 | 1 | 0 | 1 | 0 | 3 |
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
| Total | 6 | 0 | 1 | 0 | 1 | 0 | 4 |
| }} | ||
| onBeforeDelete={async (state) => { | ||
| const defaultSubAgentNodeId = defaultSubAgentNodeIdRef.current; | ||
| const defaultSubAgentNodeId = form.getValues('defaultSubAgentNodeId'); |
There was a problem hiding this comment.
🟡 Minor: Missing semicolon
Issue: This statement is missing a trailing semicolon, which violates the project's biome formatting rules ("semicolons": "always").
Why: This will cause pnpm check to fail on the format:check step.
Fix: (1-click apply)
| const defaultSubAgentNodeId = form.getValues('defaultSubAgentNodeId'); | |
| const defaultSubAgentNodeId = form.getValues('defaultSubAgentNodeId'); |
Refs:
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review (since last automated review)
The latest commit (51b99f192 polish) improves the useAgentSelectionSync hook by internalizing its dependencies:
| Before | After |
|---|---|
Hook accepted setNodes, setEdges, setQueryState as parameters |
Hook imports useAgentActions and useSidePane directly |
| Caller had to wire up and pass these dependencies | Caller only passes the data it owns (nodes, edges, isOpen, nodeId, edgeId) |
This is a clean encapsulation improvement:
- ✅ Reduces the hook's API surface from 8 parameters to 5
- ✅ Hook now owns its own side-effect dependencies
- ✅ Callers have less boilerplate
- ✅
setQueryStatecorrectly removed fromuseEffectdeps (it's stable per biome config)
No new issues introduced. The prior review's "Consider" item about memoizing selectedNode/selectedEdge lookups remains valid but is not blocking.
✅ APPROVE
Summary: Nice polish! The hook encapsulation is cleaner now — callers pass data, the hook handles its own actions. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator-manual |
1 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta-only review — sub-reviewers not dispatched due to trivial scope (parameter removal refactoring).
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review
The latest commit (f57e66fab) removes the unused setEdges destructuring from useAgentActions() in page.client.tsx. This is correct cleanup — setEdges is now called internally by the useAgentSelectionSync hook, so the parent component no longer needs it.
No new issues introduced.
✅ APPROVE
Summary: Clean removal of unused import. The refactoring is complete — ship it! 🚢
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator-delta |
1 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta-only review — sub-reviewers not dispatched due to trivial scope (unused variable removal).
|
@claude add a changeset |
|
Claude finished @dimaMachina's task in 45s —— View job Adding changeset
Added a |
Co-authored-by: Dimitri POSTOLOV <dimaMachina@users.noreply.github.com>
Ito Test Report ❌13 test cases ran. 1 failed, 12 passed. Overall, 12 of 13 test cases passed, showing strong reliability across deep-linking, authentication gating, node/edge selection synchronization, refresh/history behavior, Try It query cleanup, parallel-tab recovery, and safe handling of script-like query payloads without crashes or code-supported defects in those areas. The one confirmed issue is a medium-severity mobile usability bug where the side-pane Back button is effectively hover-only and unreliable on touch (390x844), which can trap users in node/edge detail panes and block returning to Agent view, and it appears to be pre-existing rather than introduced by this PR. ❌ Failed (1)
🟠 Mobile side pane back control is inaccessible on touch
Relevant code:
function SidePaneBackButton({ onClick }: { onClick: () => void }) {
return (
<Button
variant="ghost"
size="icon-sm"
onClick={onClick}
className="-ml-1 transition-all duration-300 ease-in-out absolute left-0 opacity-0 -translate-x-6 group-hover:opacity-100 group-hover:translate-x-0 text-foreground hover:text-foreground dark:text-foreground dark:hover:text-foreground"
>
<ArrowLeft className="h-4 w-4" />
</Button>
);
}
const showBackButton = selectedNode || selectedEdge;
return (
<SidePaneLayout.Root>
<SidePaneLayout.Header>
<div className="flex items-center relative">
{showBackButton && <SidePaneLayout.BackButton onClick={onBackButtonClick} />}
<Heading
heading={heading}
Icon={HeadingIcon}
className={cn(
showBackButton && 'transition-all duration-300 ease-in-out group-hover:translate-x-8'
)}
/>
</div>
backToAgent() {
clearCanvasSelection();
setQueryState({
pane: 'agent',
nodeId: null,
edgeId: null,
});
}✅ Passed (12)
Commit: Tell us how we did: Give Ito Feedback |
1 participant
No description provided.