Conversation
Test & Lint Summary
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the progress—this gets us close, but a couple of gaps still block approval:
- The entity form never validates for duplicate edges, so users can create the same connection multiple times despite the spec requiring us to block duplicates.
- The list pages only support search; there is no Title/Template sort even though that was part of the acceptance criteria for these tables.
I left inline notes (plus a Storybook nit). Please address these so the new list UI matches the agreed behavior.
| })) | ||
| .filter((conn) => conn.source && conn.sourceHandle && conn.targetHandle) ?? []; | ||
|
|
||
| const payload: GraphEntityUpsertInput = { |
There was a problem hiding this comment.
[major] Issue #1286 explicitly called for blocking duplicate edges when editing entities, but currently just filters out empty rows and allows the same connection tuple to be sent multiple times. That produces duplicated graph edges (e.g., two identical connections) with no user feedback. Please add client-side validation before building so we detect duplicates per direction (outgoing: target node + source/target handles, incoming: source node + source/target handles), surface an error on the offending row(s), and abort the submit until the duplicates are resolved.
| const templates = useMemo(() => getTemplateOptions(templatesQuery.data ?? [], kind), [kind, templatesQuery.data]); | ||
| const connections = useMemo(() => toConnectionList(graphQuery.data?.edges), [graphQuery.data?.edges]); | ||
|
|
||
| const filteredEntities = useMemo(() => { |
There was a problem hiding this comment.
[major] The list pages only filter entities; there is no sorting at all. The issue spec called out Title/Template sorts as part of the common table pattern, but filteredEntities simply returns the graph order and EntityTable renders the array as-is. Please add explicit sort state (e.g., Title asc/desc, Template asc/desc) with UI controls so users can reorder the list per the acceptance criteria, and ensure the rows are sorted before passing them to the table.
| screen: { | ||
| routePath: '/agents/memory', | ||
| initialEntry: '/agents/memory', | ||
| routePath: '/memory', |
There was a problem hiding this comment.
[nit] The Memory routes now sit under the new entitiesMemory menu item, but the Storybook screen parameters still pass selectedMenuItem: 'memory'. That means the sidebar highlighting in the story no longer matches real navigation. Please update this (and the pages/AgentsMemoryManager.stories.tsx story) to reference the new entitiesMemory id so the documentation stays in sync with the app.
| tags: ['smoke'], | ||
| parameters: { | ||
| layout: 'fullscreen', | ||
| screen: { |
There was a problem hiding this comment.
[nit] Same note here—the sidebar item id is now entitiesMemory, but the story still sets selectedMenuItem: 'memory'. Updating this keeps the storybook navigation highlighting aligned with the app.
|
Updates pushed:
Tests:
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the fast follow-up—sorting/test coverage and the Storybook nav fixes look great. However, the duplicate-edge UX still doesn’t tell the user which row is invalid; the earlier request specifically called for inline errors on the offending connection rows. Please flag the specific row fields when duplicates are detected so users can fix them without guesswork.
| connections: [...outgoingConnections, ...incomingConnections], | ||
| }; | ||
|
|
||
| if (payload.connections.length > 0) { |
There was a problem hiding this comment.
[major] Thanks for adding duplicate detection, but the UX requirement wasn't addressed: the prior feedback asked to "surface an error on the offending row(s)" so the user knows which connection(s) to fix. Right now every duplicate produces a generic alert with no per-row signal, so on a large graph the user still has to guess which entry is invalid. Please flag the specific row fields (e.g., outgoing.${index}.targetNodeId / incoming.${index}.sourceNodeId) as invalid when you detect a duplicate so the form renders inline errors next to the problematic connection. That keeps the guidance actionable and matches the earlier request.
|
Inline duplicate validation update:
Tests:
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Looks good—duplicate connections now trigger inline field errors, sorting controls remain intact, and supporting tests cover both behaviors. Thanks for the quick iteration.
|
CI is green and internal review is approved (Noa). Merge is currently blocked by branch protection due to a pending review request for the team |
Test & Lint Summary
|
Test & Lint Summary (updated)
|
|
[nit] The component-level MemoryManager story (packages/platform-ui/stories/MemoryManager.stories.tsx) still uses the legacy sidebar id 'memory'. Please switch it to 'agentsMemory' so Storybook highlights the same nav item the app now uses. |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for restoring the Agents menu and adding the new memory list—the routing/tests look good. A couple of follow-ups before merge:
- The sidebar never expands the Entities section even when the current location (e.g., /agents) lives under it, so the nav renders with no visible active item on page load. Please auto-expand the section that owns
selectedMenuItemso the current page is reflected in the UI. - Storybook still highlights the Entities memory nav item (and even the legacy
memoryid) for the Agents memory manager stories. Please point those stories back at/agents/memorywithselectedMenuItem: 'agentsMemory', and update the component-level story to use the new id as well so our docs mirror production.
| selectedMenuItem = 'entitiesAgents', | ||
| onMenuItemSelect | ||
| }: SidebarProps) { | ||
| const [expandedItems, setExpandedItems] = useState<Set<string>>(new Set(['agents'])); |
There was a problem hiding this comment.
[major] With the Agents directory now living under the new Entities section, /agents is our landing page and selectedMenuItem defaults to entitiesAgents. However, the sidebar still initializes expandedItems to new Set(['agents']) and never auto-expands the section that contains the active item. As a result, the app bootstraps on /agents with the Entities section collapsed, so there is no visible nav highlight anywhere in the sidebar even though a page is selected. Please derive the initial (and ideally subsequent) expansion set from selectedMenuItem so that whichever top-level group contains the active entry is expanded by default. Otherwise the navigation no longer reflects the current location.
| tags: ['smoke'], | ||
| parameters: { | ||
| layout: 'fullscreen', | ||
| screen: { |
There was a problem hiding this comment.
[nit] The Agents memory manager now lives at /agents/memory under the agentsMemory nav item, but this story still pretends it resides under the Entities section (selectedMenuItem: 'entitiesMemory' with /memory routes). Please point the Storybook routePath/initialEntry at /agents/memory again and set selectedMenuItem to agentsMemory so the docs mirror the real navigation.
| component: MemoryManager, | ||
| decorators: [withMainLayout], | ||
| parameters: { | ||
| layout: 'fullscreen', |
There was a problem hiding this comment.
[nit] Same issue here—the screen story still highlights entitiesMemory and navigates to /memory, but the real memory manager UX is back under /agents/memory (agentsMemory nav item). Please update the Storybook parameters so the sidebar in docs matches the actual app again.
Test & Lint Summary (updated)
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Sidebar now expands the active section automatically, and all memory-related Storybook entries point at /agents/memory with the correct nav id. Thanks for turning those around quickly.
Local Validation
Did you mean "pnpm lint"?
Results:
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up—the Entities pages now share the same Button/Select/Dialog primitives as the rest of the app, and the tests were adjusted for the native selects. Everything looks aligned.
|
Summary:
Validation:
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Refactor looks solid: Dropdown now piggybacks on the shared SelectInput, ReferenceInput uses the segmented control without regressing focus/autocomplete behavior, and the test suite was updated for the native selects. Thanks for cleaning up the old Radix shims.
|
Product scope update per owner directive:
Next steps from review:
|
|
Addressed Noa's feedback:
Verification:
Results: lint ✅ · typecheck ✅ · tests ✅ (411 passed) |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up. The embedded node-properties view now receives a real node id (or a stable preview id in create mode), so prompt previews/context-aware helpers work as expected, and the unused formTypes module has been removed. Also acknowledged the scope update on #1286 re: edge editing. LGTM.
|
Reverted the sidebar integration work to match
Validation:
Results: lint ✅ · typecheck ✅ · tests ✅ (411 passed) |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Reverting to the JSON form is fine, but we shouldn’t merge dead helpers. Please drop the now-unused entity hooks before we land this.
packages/platform-ui/src/components/entities/EntityFormDialog.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,106 @@ | |||
| import { useCallback, useRef, useState } from 'react'; | |||
There was a problem hiding this comment.
[minor] Now that the entity dialog no longer embeds nodeProperties, nothing calls this hook anymore (repo-wide search only finds this file). Please delete it (and the matching useNixServices helper) instead of shipping dead code we can’t exercise.
| @@ -0,0 +1,60 @@ | |||
| import { useMemo } from 'react'; | |||
There was a problem hiding this comment.
[minor] Same story here—this helper isn’t referenced anywhere after swapping the dialog back to plain JSON inputs. Please drop it so we don’t merge unused code.
|
Removed the unused entity hooks:
Verification (per request):
Results: typecheck ✅ · tests ✅ (411 passed) |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks—confirming the now-unused entity helper hooks were removed after the nodeProperties rollback. No further issues from my side.
|
Tests & lint summary:
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Embedding the node-properties view is a good step, but the dialog currently renders those components without any node or graph context. That breaks the prompt previews and managed-agent helpers that rely on /graph data, so the list dialog no longer matches the graph sidebar.
Please thread the entity id (or a deterministic preview id) plus the loaded graph nodes/edges into and pass them along to each view so the context-aware behavior stays intact.
| } | ||
| switch (nodeKind) { | ||
| case 'Trigger': { | ||
| const triggerProps: NodePropertiesViewProps<'Trigger'> = { |
There was a problem hiding this comment.
[major] These node-properties views need the same context they get in the graph sidebar (, , , etc.), otherwise all of the context-aware affordances go dark. For example, bails out of building the managed-agent preview unless a node id is provided, and cannot resolve upstream prompts without the node/edge lists. In edit mode we already know and the loaded graph lives in , so we can pass the real ids/graph data through. In create mode we previously used a deterministic placeholder id so prompt previews still render. Please thread those props down (and update the view props here) so the embedded experience truly matches the sidebar; right now agent/tool previews regress to empty output in the list dialog.}
| </div> | ||
| </div> | ||
|
|
||
| <EntityFormDialog |
There was a problem hiding this comment.
[major] To satisfy the "embed the same config view as the graph sidebar" requirement we also have to provide the same data those views consume. Right now the dialog never receives a node id or the surrounding graph nodes/edges, so anything that relies on graph context (managed-agent prompt previews, agent prompt resolver, etc.) silently degrades. Since already exposes the loaded graph, please pass the nodes/edges (and the current entity id or a deterministic preview id) into the dialog so it can forward them to the views. Otherwise the list dialog can’t actually mirror the sidebar behavior.
| </div> | ||
| </div> | ||
|
|
||
| <EntityFormDialog |
There was a problem hiding this comment.
[major] To satisfy the 'embed the same config view as the graph sidebar' requirement we also have to provide the same data those views consume. Right now the dialog never receives a node id or the surrounding graph nodes/edges, so anything that relies on graph context (managed-agent prompt previews, agent prompt resolver, etc.) silently degrades. Since useGraphEntities() already exposes the loaded graph, please pass the nodes/edges (and the current entity id or a deterministic preview id) into the dialog so it can forward them to the views. Otherwise the list dialog can’t actually mirror the sidebar behavior.
|
Tests & Lint Summary
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Appreciate the follow-up—confirming the dialog now threads the node id plus the full graph context into the embedded node-properties views, and the list page supplies that data from the loaded graph. Managed-agent previews and other context-aware affordances work again. No further issues from my side.
|
Tests & Lint Summary
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Looks good—header/filter/tooling now mirror the Secrets layout, and the updated table implementation adds the sticky header plus row styling without changing behavior. No additional issues found.
|
Additional Tests & Lint Summary
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Confirmed the old shadcn table wrapper was removed and the remaining consumers (Memory list + entity table) now use the same native markup/styling as the Secrets page. No regressions spotted.
|
Additional Tests & Lint Summary
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up—confirmed the list pages now match the Secrets layout (no outer padding, no search row), the dialog header/footer stay fixed while only the config scrolls, node-properties views resolve via the shared registry with full graph context, and the memory list mirrors the same table treatment. All tests updated accordingly.
3 participants
Summary
Fixes #1286
Testing