-
Notifications
You must be signed in to change notification settings - Fork 45k
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
WIP: Full Library Integration PR #9407
base: dev
Are you sure you want to change the base?
WIP: Full Library Integration PR #9407
Conversation
β¦re-agents-without-agent-ownership
β¦without-agent-ownership' of github.com:Significant-Gravitas/AutoGPT into swiftyos/open-2276-add-ability-to-execute-store-agents-without-agent-ownership
β¦re-agents-without-agent-ownership
Co-authored-by: Reinier van der Leer <[email protected]>
Co-authored-by: Reinier van der Leer <[email protected]>
β¦re-agents-without-agent-ownership
β¦without-agent-ownership' into swiftyos/open-2277-implement-library-add-update-remove-archive-functionality
β¦archive-functionality' into swiftyos/open-2278-implement-agent-preset-functionality
β¦lement-agent-preset-functionality
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
β¦lement-new-agent-library-to-replace-monitor-page
Conflicts have been resolved! π A maintainer will review the pull request shortly. |
β¦lement-new-agent-library-to-replace-monitor-page
β¦lement-new-agent-library-to-replace-monitor-page
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
detail=f"Store listing version {store_listing_version_id} not found", | ||
) from exc | ||
|
||
except backend.server.v2.store.exceptions.DatabaseError as exc: |
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.
??? make a generic exceptions folder probs? why are we cross importing
@staticmethod | ||
async def test_get_presets(user_id: str, page: int = 1, page_size: int = 10): | ||
return await backend.server.v2.library.routes.presets.get_presets( | ||
user_id=user_id, page=page, page_size=page_size | ||
) |
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.
why are we mounting all of these here?
@@ -309,11 +310,6 @@ async def get_graph( | |||
tags=["graphs"], | |||
dependencies=[Depends(auth_middleware)], | |||
) | |||
@v1_router.get( |
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.
if we're dropping the routers can we just delete the code?
@@ -502,6 +523,7 @@ def execute_graph( | |||
graph_version: Optional[int] = None, | |||
) -> dict[str, Any]: # FIXME: add proper return type | |||
try: | |||
logger.info("Node input: %s", node_input) |
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.
probs not needed and highly likely to leak personal data
@@ -688,7 +669,7 @@ async def create_api_key( | |||
) | |||
return CreateAPIKeyResponse(api_key=api_key, plain_text_key=plain_text) | |||
except APIKeyError as e: | |||
logger.error(f"Failed to create API key: {str(e)}") | |||
logger.error("Failed to create API key: %s", e) |
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.
why all these changes?
debouncedSearch(searchTerm); | ||
}; | ||
|
||
return ( |
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.
is this fundamentally different than the other search bar? why have two? can we push back on design to keep a subset of components and move faster
const updatedBlockIDMap: Record<string, string> = { | ||
"a1b2c3d4-5e6f-7g8h-9i0j-k1l2m3n4o5p6": | ||
"436c3984-57fd-4b85-8e9a-459b356883bd", | ||
"b2g2c3d4-5e6f-7g8h-9i0j-k1l2m3n4o5p6": | ||
"0e50422c-6dee-4145-83d6-3a5a392f65de", | ||
"c3d4e5f6-7g8h-9i0j-1k2l-m3n4o5p6q7r8": | ||
"a0a69be1-4528-491c-a85a-a4ab6873e3f0", | ||
"c3d4e5f6-g7h8-i9j0-k1l2-m3n4o5p6q7r8": | ||
"32a87eab-381e-4dd4-bdb8-4c47151be35a", | ||
"b2c3d4e5-6f7g-8h9i-0j1k-l2m3n4o5p6q7": | ||
"87840993-2053-44b7-8da4-187ad4ee518c", | ||
"h1i2j3k4-5l6m-7n8o-9p0q-r1s2t3u4v5w6": | ||
"d0822ab5-9f8a-44a3-8971-531dd0178b6b", | ||
"d3f4g5h6-1i2j-3k4l-5m6n-7o8p9q0r1s2t": | ||
"df06086a-d5ac-4abb-9996-2ad0acb2eff7", | ||
"h5e7f8g9-1b2c-3d4e-5f6g-7h8i9j0k1l2m": | ||
"f5b0f5d0-1862-4d61-94be-3ad0fa772760", | ||
"a1234567-89ab-cdef-0123-456789abcdef": | ||
"4335878a-394e-4e67-adf2-919877ff49ae", | ||
"f8e7d6c5-b4a3-2c1d-0e9f-8g7h6i5j4k3l": | ||
"f66a3543-28d3-4ab5-8945-9b336371e2ce", | ||
"b29c1b50-5d0e-4d9f-8f9d-1b0e6fcbf0h2": | ||
"716a67b3-6760-42e7-86dc-18645c6e00fc", | ||
"31d1064e-7446-4693-o7d4-65e5ca9110d1": | ||
"cc10ff7b-7753-4ff2-9af6-9399b1a7eddc", | ||
"c6731acb-4105-4zp1-bc9b-03d0036h370g": | ||
"5ebe6768-8e5d-41e3-9134-1c7bd89a8d52", | ||
}; |
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.
wtf is this? we gotta explain this with a comment or something and also should this be happening on the backend?
@@ -0,0 +1,35 @@ | |||
"use client"; |
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.
does this need to be a client compoentnet
@@ -0,0 +1,22 @@ | |||
"use client"; |
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.
does this need to be a client component
} | ||
|
||
async addAgentToLibrary(storeListingVersionId: string): Promise<void> { | ||
console.log("Adding to the library"); |
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.
?
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.
I've taken a look at only a few of the components to start here. I'd highly suggest taking a step back and keeping a few things in mind while working with these components.
Let's focus on having as little client components as possible. Think of our project as a tree where only the leaves should be client components. When building the pages and components we should continuously ask ourselves does our ui components we get from ShadcN already cover this and if so how can I use them.
One other thing to keep in mind is how React handles re-renders. A good bit of what is getting tracked with useState
and other items I've seen here can simply be removed and handled by updating props. When React detects that the props flowing into the components have changed they will re-render.
@@ -0,0 +1,35 @@ | |||
"use client"; |
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.
The "use client" directive is being used here primarily because of the imported interactive components, but the main component structure is static layout.
The layout structure could be server-rendered, Only the individual interactive components (SearchBar, NotificationDropdown, UploadAgent) need to be client components
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.
This layout component is unnecessarily marked as a client component.
Layouts in Next.js should typically be server components unless there's a specific need for client-side interactivity.
@@ -0,0 +1,8 @@ | |||
"use client"; |
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.
This is another layout component is unnecessarily marked as a client component. Please understand that layout components should almost always be server. If you see a need for "use client" this is a key indicator of an anitpattern and should raise concerns for you.
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.
Good use of ShadcN Avatar component, but we could leverage more ShadcN components:
- Consider using ShadcN Card component as the base instead of custom div
- The hover effects could be standardized with ShadcN's hover utilities
The custom component here is labeled as a Card but you have yet to actually use a Card Component?
@@ -0,0 +1,63 @@ | |||
import { LibraryAgentSortEnum } from "@/lib/autogpt-server-api"; |
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.
The setTimeout in handleSortChange seems unnecessary - we should let React's natural state updates handle loading states
Id suggest checking into how React, and NextJS handle loading states.
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.
For this component I'd suggest a bit of a refinement in your approach. Please keep these things in mind moving forward with your components.
- Replace custom card with ShadCN Card components
- Utilize ShadCN's built-in variants for different notification types
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.
Looking at your Dropdown here it seems a bit overly complex. I think we could simplify this component quite a bit.
Let's focus on what the dropdown really needs:
- A trigger button showing notification count
- A dropdown menu
- A list of notification items
- Basic dismiss functionality
Start by removing the unnecessary features:
- Remove Framer Motion animations
- Remove custom scroll handling
- Remove local state management for notifications
- The initialNotificationData memo is unnecessary
- Double state tracking for open isn't needed with ShadCN's DropdownMenu
- Use ShadCN's built-in positioning and styling
Use Shad Cn Components:
- DropdownMenu (already using)
- Button with badge for notification count
- ScrollArea for scrollable content
- Toast for notifications instead of custom cards
Changes ποΈ
Checklist π
For code changes:
Example test plan
For configuration changes:
.env.example
is updated or already compatible with my changesdocker-compose.yml
is updated or already compatible with my changesExamples of configuration changes