-
Notifications
You must be signed in to change notification settings - Fork 46.2k
feat(platform): Builder search history #11457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for auto-gpt-docs canceled.
|
|
Here's the code health analysis summary for commits Analysis Summary
|
|
Thank you for your PR! Before we can proceed with merging, there are a few issues that need to be addressed:
Once you've updated these items, we can proceed with a more thorough review of your implementation. The PR title looks good with the proper conventional commit format. |
|
Thank you for your contribution! There are a few items that need to be addressed before this PR can be merged:
Once these issues are addressed, we can proceed with the review. The code change itself looks straightforward, but we need proper documentation to understand its purpose. |
987baa9 to
b3c3f9c
Compare
|
Thank you for your PR contribution. Before this can be merged, please update the following:
Once you've addressed these points, please request another review. |
|
Thank you for implementing the builder search history functionality! The code implementation looks good, but there are a few things that need to be addressed before this can be merged:
The implementation itself looks solid - you've properly:
Please update the PR description and checklist, and this should be ready for review again. |
|
Thank you for your PR implementing builder search history. The code looks well-structured and focused on the stated purpose, but there are a few issues to address before this can be approved: Missing Items
Code ReviewThe code itself looks good overall:
Please update your PR description with:
Once these are addressed, the PR should be ready for approval. |
|
Thank you for your PR implementing the builder search history feature. Before this can be merged, please update the PR description with the following:
The code implementation looks good and matches the scope stated in the title. I can see you've:
Once you update the PR description with the missing information, we can proceed with the review process. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
kcze
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments
| SearchResultItem = BlockInfo | library_model.LibraryAgent | store_model.StoreAgent | ||
|
|
||
|
|
||
| @dataclass |
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 used private @dataclass because are more lightweight than pydantic model as this goes to cache
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.
Sure, but any idea how much more lightweight and how this affects performance?
| return False | ||
|
|
||
|
|
||
| def _score_block( |
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.
Scoring uses magic numbers for priorities - this need to be refined over time, so search works as best as possible.
...latform)/build/components/NewControlPanel/NewBlockMenu/BlockMenuSearch/useBlockMenuSearch.ts
Outdated
Show resolved
Hide resolved
...m)/build/components/NewControlPanel/NewBlockMenu/BlockMenuSearchBar/useBlockMenuSearchBar.ts
Outdated
Show resolved
Hide resolved
...tform)/build/components/NewControlPanel/NewBlockMenu/SuggestionContent/SuggestionContent.tsx
Outdated
Show resolved
Hide resolved
ntindle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend looks fine, have another eval front end
| @@ -0,0 +1,15 @@ | |||
| -- CreateTable | |||
| CREATE TABLE "BuilderSearch" ( | |||
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.
can be more specific:
BuilderSearchHistory
| CREATE TABLE "BuilderSearch" ( | ||
| "id" TEXT NOT NULL, | ||
| "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| "updatedAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, |
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.
are these records ever updated?
Preserve user searches in the new builder and cache search results for more efficiency.
Search is saved, so the user can see their previous searches.
Changes 🏗️
BuilderSearchcolumn&migration to save user search (with all filters)db.pynow caches all search results using@cachedand returns paginated results, so following pages are returned much quickerserachIdand use it for subsequent searches, so we don't save partial searches (e.g. "b", "bl", ..., "block"). Search id is reset when user clears the search field.HorizontalScrollcomponent (chips use it)Checklist 📋
For code changes: