Integrate create collection #293
Conversation
|
scroll-bar befaves strange |
There was a problem hiding this comment.
Pull Request Overview
This PR integrates the "create collection" feature into the application, including theming, UI components, creation logic, and tests.
- Added theme overrides for the CreateCollectionForm in both light and dark themes
- Integrated a CreateCollectionButton and dialog into the Collections page layout
- Implemented collection-creation logic (
create-collection.js), its tests, and added thecreate-collection-formdependency
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/theme/light-theme.js | Added MuiCreateCollectionForm style overrides for the light theme |
| src/theme/dark-theme.js | Added MuiCreateCollectionForm style overrides for the dark theme |
| src/pages/Collections.jsx | Imported and placed CreateCollectionButton, updated layout and filter callback |
| src/components/Collections/CreateCollection/create-collection.js | Implemented parameter builders and createCollection function |
| src/components/Collections/CreateCollection/create-collection.test.js | Added unit tests for configuration generation and creation logic |
| src/components/Collections/CreateCollection/CreateCollectionDialog.jsx | New full-screen dialog component for the create-collection form |
| src/components/Collections/CreateCollection/CreateCollectionButton.jsx | New button component that opens the create-collection dialog |
| package.json | Added "create-collection-form" dependency |
Comments suppressed due to low confidence (2)
src/components/Collections/CreateCollection/CreateCollectionDialog.jsx:69
- The attribute
aria-roleis invalid. You should use the standardrole="dialog"attribute to properly convey the dialog semantics to assistive technologies.
aria-role="dialog"
src/pages/Collections.jsx:72
- Filtering uses
collections, which may already be filtered. To always search over the full set, use the unfilteredrawCollectionsarray instead.
const filteredCollections = collections.filter((collection) => collection.name.match(query));
| size: size, | ||
| distance, | ||
| multivector_config: multivectorConfig, | ||
| on_disk: !originalInRam, |
There was a problem hiding this comment.
The on_disk flag is inverted against originalInRam. With storageTier === STORAGE, originalInRam is set to false but on_disk becomes true, contradicting test expectations. Consider using on_disk: originalInRam or adjusting the storage logic to match the intended behavior.
| on_disk: !originalInRam, | |
| on_disk: originalInRam, |
Copilot uses AI. Check for mistakes.
| for (const fieldConfig of params.payload_indexes) { | ||
| const fieldName = fieldConfig.name; | ||
| const fieldSchema = { | ||
| type: fieldConfig.type, | ||
| ...(fieldConfig.params || {}), | ||
| }; | ||
| await qdrantClient.createPayloadIndex(collectionName, { field_name: fieldName, field_schema: fieldSchema }); | ||
| } |
There was a problem hiding this comment.
Creating each payload index sequentially with await can be slow. Consider collecting the calls into an array of promises and using await Promise.all(...) to run them in parallel for better throughput.
| for (const fieldConfig of params.payload_indexes) { | |
| const fieldName = fieldConfig.name; | |
| const fieldSchema = { | |
| type: fieldConfig.type, | |
| ...(fieldConfig.params || {}), | |
| }; | |
| await qdrantClient.createPayloadIndex(collectionName, { field_name: fieldName, field_schema: fieldSchema }); | |
| } | |
| const payloadIndexPromises = params.payload_indexes.map((fieldConfig) => { | |
| const fieldName = fieldConfig.name; | |
| const fieldSchema = { | |
| type: fieldConfig.type, | |
| ...(fieldConfig.params || {}), | |
| }; | |
| return qdrantClient.createPayloadIndex(collectionName, { field_name: fieldName, field_schema: fieldSchema }); | |
| }); | |
| await Promise.all(payloadIndexPromises); |
Copilot uses AI. Check for mistakes.
| configuration.payload_indexes.push({ | ||
| name: configuration.tenant_field.name, | ||
| type: configuration.tenant_field.type, | ||
| params: { | ||
| on_disk: false, | ||
| is_tenant: true, | ||
| is_principal: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
This mutates the configuration object by pushing a new tenant field into payload_indexes. To avoid side effects, clone payload_indexes (e.g., using spread syntax) before appending.
| configuration.payload_indexes.push({ | |
| name: configuration.tenant_field.name, | |
| type: configuration.tenant_field.type, | |
| params: { | |
| on_disk: false, | |
| is_tenant: true, | |
| is_principal: true, | |
| }, | |
| }); | |
| const updatedPayloadIndexes = [ | |
| ...configuration.payload_indexes, | |
| { | |
| name: configuration.tenant_field.name, | |
| type: configuration.tenant_field.type, | |
| params: { | |
| on_disk: false, | |
| is_tenant: true, | |
| is_principal: true, | |
| }, | |
| }, | |
| ]; | |
| configuration.payload_indexes = updatedPayloadIndexes; |
Copilot uses AI. Check for mistakes.
| MuiCreateCollectionForm: { | ||
| styleOverrides: { | ||
| root: ({ theme }) => { | ||
| return { |
There was a problem hiding this comment.
[nitpick] The dark theme override for MuiCreateCollectionForm omits the backgroundColor: 'transparent' property present in the light theme. Consider adding it for consistent styling.
| return { | |
| return { | |
| backgroundColor: 'transparent', |
Copilot uses AI. Check for mistakes.
2 participants
No description provided.