feat: move out search for grid to separate component#825
feat: move out search for grid to separate component#825
Conversation
|
/review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bdb50c0c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!submittedQueryText) { | ||
| setTextEmbedding(undefined); | ||
| return; |
There was a problem hiding this comment.
This effect clears global textEmbedding whenever submittedQueryText is empty, and after this refactor GridSearch is mounted in two mutually exclusive layout branches ($showPlot true/false). Toggling “Show Embeddings” remounts the component, so this branch runs on first render and drops an already-applied search filter unexpectedly.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,272 @@ | |||
| <script lang="ts"> | |||
There was a problem hiding this comment.
The frontend guideline in docs/coding-guidelines/frontend.md asks to keep components under 100 lines, but this component is 272 lines and mixes upload logic, clipboard/drag handlers, API calls, and rendering in one file. Breaking this into smaller components/hooks would match the project style and improve maintainability.
Useful? React with 👍 / 👎.
| let submittedQueryText = $state(''); | ||
| let previewUrl = $state<string | null>(null); | ||
| let isUploading = $state(false); | ||
| let query_text = $state($textEmbedding ? $textEmbedding.queryText : ''); |
There was a problem hiding this comment.
The frontend naming guideline requires camelCase for variables, but query_text uses snake_case. Renaming it (for example to queryText) would align this component with the code style used across the TypeScript/Svelte codebase.
Useful? React with 👍 / 👎.
| buildVideoAnnotationCountsFilter, | ||
| buildVideoFrameAnnotationCountsFilter | ||
| } from '$lib/utils/buildAnnotationCountsFilters'; | ||
| import GridSearch from '$lib/components/GridSearch/GridSearch.svelte'; |
There was a problem hiding this comment.
The frontend guidelines recommend importing project-specific components from $lib/components; this direct path import bypasses the barrel even though GridSearch is exported there in the same commit. Using the barrel keeps imports consistent and easier to refactor.
Useful? React with 👍 / 👎.
| toast.error('Error', { description: errorMessage }); | ||
| }; | ||
|
|
||
| async function uploadImage(file: File) { |
There was a problem hiding this comment.
I'd expect this function in a hook. WDTY?
LeonardoRosaa
left a comment
LeonardoRosaa
left a comment
There was a problem hiding this comment.
Can you write unit tests for this new component?
2 participants
What has changed and why?
This PR extracts the GridSearch component from the layout to reduce the complexity of the layout and code duplication.
There are no logic changes - just moving the component to the separate file and cleaning up the code duplication.
How has it been tested?
Ny e2e tests and visually by running
make start-e2eand checking what search is still working after the refactoringDid you update CHANGELOG.md?