feat(search): show facets count#210
Conversation
|
Related to plone/plone.restapi#1637 |
|
@greptile |
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive facets count feature for the Volto search block system, allowing users to see the number of results available for each facet option (e.g., "Environment (5)", "Climate (12)"). The implementation spans the entire search architecture, from Redux state management to UI components.
The core enhancement starts with the querystringsearch reducer customization, which extends Volto's base reducer to store facets_count data alongside search results. This data comes from the backend API and is preserved throughout all action handlers (SUCCESS, FAIL, RESET) for both main requests and subrequests.
Three new Higher-Order Components (HOCs) are introduced to manage different aspects of search functionality: withFacetsCount connects components to the Redux store to access facet count data, withQueryString fetches querystring metadata needed for search operations, and withSearch provides comprehensive search state management including URL synchronization and facet handling.
The UI implementation includes new layout components (LeftColumnFacets, RightColumnFacets, TopSideFacets) that support different positioning of facets while consistently passing count data through props. Individual facet components (CheckboxFacet, SelectFacet) are customized to conditionally display counts in their labels when the feature is enabled, maintaining backward compatibility.
The architecture uses a composition pattern where the main SearchBlockEdit and SearchBlockView components are wrapped with multiple HOCs to inject search functionality, querystring metadata, and facet count data. A utility module provides helper functions for categorizing different types of querystring operations, which is essential for proper facet count calculation.
This feature integrates seamlessly with existing Volto patterns and maintains full backward compatibility, only displaying counts when the data is available from the backend API.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
| src/customizations/volto/reducers/querystringsearch/querystringsearch.js | 5/5 | Complete reducer customization adding facets_count support to all action handlers |
| src/customizations/volto/components/manage/Blocks/Search/hocs/index.js | 5/5 | Central export point for three search-related HOCs |
| src/customizations/volto/components/manage/Blocks/Search/layout/TopSideFacets.jsx | 5/5 | Top-positioned facets layout with facetsCount prop integration |
| src/customizations/volto/components/manage/Blocks/Search/hocs/withQueryString.jsx | 4/5 | HOC for fetching and injecting querystring metadata |
| src/customizations/volto/components/manage/Blocks/Search/hocs/withFacetsCount.jsx | 5/5 | HOC providing facets count data from Redux store |
| src/customizations/volto/components/manage/Blocks/Search/layout/RightColumnFacets.jsx | 5/5 | Right-column facets layout with count display support |
| src/customizations/volto/components/manage/Blocks/Search/SearchBlockEdit.jsx | 4/5 | Main search edit component with integrated HOCs and automatic search triggering |
| src/customizations/volto/components/manage/Blocks/Listing/withQuerystringResults.jsx | 3/5 | HOC for listing blocks with querystring results and potential performance issues |
| src/customizations/volto/components/manage/Blocks/Search/layout/LeftColumnFacets.jsx | 4/5 | Left-column facets layout with consistent prop passing patterns |
| src/customizations/volto/components/manage/Blocks/Search/components/SelectFacet.jsx | 3/5 | Select facet with conditional count display but duplicated component logic |
| src/customizations/volto/components/manage/Blocks/Search/components/Facets.jsx | 3/5 | Main facets component with potential logic issues in choice filtering |
| src/customizations/volto/components/manage/Blocks/Search/utils.js | 4/5 | Utility functions for categorizing querystring operations |
| src/customizations/volto/components/manage/Blocks/Search/components/CheckboxFacet.jsx | 4/5 | Checkbox facet with conditional count display using duplicated logic |
| src/customizations/volto/components/manage/Blocks/Search/SearchBlockView.jsx | 4/5 | Main search view component with HOC composition and memoization |
| src/customizations/volto/components/manage/Blocks/Search/hocs/withSearch.jsx | 3/5 | Complex search state management HOC with potential performance and security issues |
Confidence score: 3/5
- This PR requires careful review due to complex state management logic and potential performance issues
- Score lowered due to duplicated component logic, complex useEffect dependencies, and potential filtering bugs that could hide valid facet options
- Pay close attention to withSearch.jsx, Facets.jsx, and the facet component files with duplicated conditional rendering logic
Sequence Diagram
sequenceDiagram
participant User
participant SearchBlockEdit
participant SearchBlockView
participant withFacetsCount
participant withQueryString
participant withSearch
participant Redux
participant Backend
participant Facets
participant CheckboxFacet
participant SelectFacet
User->>SearchBlockEdit: "Configures search block"
SearchBlockEdit->>withQueryString: "Gets querystring metadata"
withQueryString->>Redux: "Dispatches getQuerystring()"
Redux->>Backend: "Fetches querystring indexes"
Backend-->>Redux: "Returns querystring data"
Redux-->>withQueryString: "Provides querystring state"
User->>SearchBlockView: "Views search results"
SearchBlockView->>withSearch: "Triggers search"
withSearch->>Redux: "Dispatches getQueryStringResults()"
Redux->>Backend: "Fetches search results with facets"
Backend-->>Redux: "Returns items and facets_count"
Redux-->>withSearch: "Updates querystringsearch state"
withFacetsCount->>Redux: "Selects facets_count from state"
Redux-->>withFacetsCount: "Returns facetsCount data"
withFacetsCount->>Facets: "Passes facetsCount prop"
Facets->>CheckboxFacet: "Renders with facetCount and isFacetCountEnabled"
CheckboxFacet->>User: "Shows 'Label (count)' format"
Facets->>SelectFacet: "Renders with facetCount and isFacetCountEnabled"
SelectFacet->>User: "Shows 'Label (count)' in dropdown"
User->>CheckboxFacet: "Selects facet option"
CheckboxFacet->>withSearch: "Triggers onTriggerSearch()"
withSearch->>Redux: "Updates search with new facets"
15 files reviewed, 11 comments
| initialPath, | ||
| { | ||
| ...adaptedQuery, | ||
| b_size: 10000000000, |
There was a problem hiding this comment.
style: This extremely large b_size value (10 billion) could cause performance issues and memory problems. Consider using a more reasonable limit.
| const totalPages = showAsFolderListing | ||
| ? Math.ceil(content.items_total / b_size) | ||
| : showAsQueryListing | ||
| ? Math.ceil(querystringResults[subrequestID].total / b_size) |
There was a problem hiding this comment.
logic: Potential runtime error if querystringResults[subrequestID] is undefined when showAsQueryListing is true. Add null check like the pattern used elsewhere.
| ? Math.ceil(querystringResults[subrequestID].total / b_size) | |
| ? Math.ceil(querystringResults[subrequestID]?.total / b_size) |
| const prevBatch = showAsFolderListing | ||
| ? content.batching?.prev | ||
| : showAsQueryListing | ||
| ? querystringResults[subrequestID].batching?.prev |
There was a problem hiding this comment.
logic: Same potential undefined access issue here - should check if querystringResults[subrequestID] exists before accessing batching.
| ? querystringResults[subrequestID].batching?.prev | |
| ? querystringResults[subrequestID]?.batching?.prev |
| if (isFacetCountEnabled) | ||
| return ( | ||
| <Select | ||
| placeholder={facet?.title ?? (facet?.field?.label || 'select...')} | ||
| className="react-select-container" | ||
| classNamePrefix="react-select" | ||
| options={choices} | ||
| styles={customSelectStyles} | ||
| theme={selectTheme} | ||
| components={{ DropdownIndicator, Option, MultiValueContainer }} | ||
| isDisabled={isEditMode} | ||
| onChange={(data) => { | ||
| if (data) { | ||
| onChange( | ||
| facet.field.value, | ||
| isMulti ? data.map(({ value }) => value) : data.value, | ||
| ); | ||
| } else { | ||
| // data has been removed | ||
| onChange(facet.field.value, isMulti ? [] : ''); | ||
| } | ||
| }} | ||
| isMulti={facet.multiple} | ||
| isClearable | ||
| value={v} | ||
| getOptionLabel={({ label, value }) => { | ||
| return `${label} (${facetCount?.data?.[value] || 0})`; | ||
| }} | ||
| /> | ||
| ); | ||
| else | ||
| return ( | ||
| <Select | ||
| placeholder={facet?.title ?? (facet?.field?.label || 'select...')} | ||
| className="react-select-container" | ||
| classNamePrefix="react-select" | ||
| options={choices} | ||
| styles={customSelectStyles} | ||
| theme={selectTheme} | ||
| components={{ DropdownIndicator, Option, MultiValueContainer }} | ||
| isDisabled={isEditMode} | ||
| onChange={(data) => { | ||
| if (data) { | ||
| onChange( | ||
| facet.field.value, | ||
| isMulti ? data.map(({ value }) => value) : data.value, | ||
| ); | ||
| } else { | ||
| // data has been removed | ||
| onChange(facet.field.value, isMulti ? [] : ''); | ||
| } | ||
| }} | ||
| isMulti={facet.multiple} | ||
| isClearable | ||
| value={v} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
style: The entire Select component is duplicated with identical logic. Consider extracting common props into a variable or using a single Select with conditional getOptionLabel prop to reduce code duplication.
| if (isFacetCountEnabled) | |
| return ( | |
| <Select | |
| placeholder={facet?.title ?? (facet?.field?.label || 'select...')} | |
| className="react-select-container" | |
| classNamePrefix="react-select" | |
| options={choices} | |
| styles={customSelectStyles} | |
| theme={selectTheme} | |
| components={{ DropdownIndicator, Option, MultiValueContainer }} | |
| isDisabled={isEditMode} | |
| onChange={(data) => { | |
| if (data) { | |
| onChange( | |
| facet.field.value, | |
| isMulti ? data.map(({ value }) => value) : data.value, | |
| ); | |
| } else { | |
| // data has been removed | |
| onChange(facet.field.value, isMulti ? [] : ''); | |
| } | |
| }} | |
| isMulti={facet.multiple} | |
| isClearable | |
| value={v} | |
| getOptionLabel={({ label, value }) => { | |
| return `${label} (${facetCount?.data?.[value] || 0})`; | |
| }} | |
| /> | |
| ); | |
| else | |
| return ( | |
| <Select | |
| placeholder={facet?.title ?? (facet?.field?.label || 'select...')} | |
| className="react-select-container" | |
| classNamePrefix="react-select" | |
| options={choices} | |
| styles={customSelectStyles} | |
| theme={selectTheme} | |
| components={{ DropdownIndicator, Option, MultiValueContainer }} | |
| isDisabled={isEditMode} | |
| onChange={(data) => { | |
| if (data) { | |
| onChange( | |
| facet.field.value, | |
| isMulti ? data.map(({ value }) => value) : data.value, | |
| ); | |
| } else { | |
| // data has been removed | |
| onChange(facet.field.value, isMulti ? [] : ''); | |
| } | |
| }} | |
| isMulti={facet.multiple} | |
| isClearable | |
| value={v} | |
| /> | |
| ); | |
| const commonProps = { | |
| placeholder: facet?.title ?? (facet?.field?.label || 'select...'), | |
| className: "react-select-container", | |
| classNamePrefix: "react-select", | |
| options: choices, | |
| styles: customSelectStyles, | |
| theme: selectTheme, | |
| components: { DropdownIndicator, Option, MultiValueContainer }, | |
| isDisabled: isEditMode, | |
| onChange: (data) => { | |
| if (data) { | |
| onChange( | |
| facet.field.value, | |
| isMulti ? data.map(({ value }) => value) : data.value, | |
| ); | |
| } else { | |
| // data has been removed | |
| onChange(facet.field.value, isMulti ? [] : ''); | |
| } | |
| }, | |
| isMulti: facet.multiple, | |
| isClearable: true, | |
| value: v, | |
| }; | |
| const getOptionLabel = isFacetCountEnabled | |
| ? ({ label, value }) => `${label} (${facetCount?.data?.[value] || 0})` | |
| : undefined; | |
| return ( | |
| <Select | |
| {...commonProps} | |
| getOptionLabel={getOptionLabel} | |
| /> | |
| ); |
| ).filter(({ _, value }) => { | ||
| if (isFacetCountEnabled === false) return true; | ||
| return Object.keys(facetCount?.data || {}).find( | ||
| (key) => key === value, | ||
| ); | ||
| })} |
There was a problem hiding this comment.
logic: Logic issue: this filter will hide choices that have zero count but should still be visible. The find() method returns undefined for zero counts, hiding valid options.
| ).filter(({ _, value }) => { | |
| if (isFacetCountEnabled === false) return true; | |
| return Object.keys(facetCount?.data || {}).find( | |
| (key) => key === value, | |
| ); | |
| })} | |
| ).filter(({ _, value }) => { | |
| if (isFacetCountEnabled === false) return true; | |
| return Object.prototype.hasOwnProperty.call( | |
| facetCount?.data || {}, | |
| value | |
| ); | |
| })} |
| export const hasDateOperation = (ops) => { | ||
| return ops.filter((x) => DATE_OPERATIONS.has(x)).length > 0; | ||
| }; |
There was a problem hiding this comment.
style: Consider using some() instead of filter().length > 0 for better performance when checking if any operation exists in the set
| export const hasDateOperation = (ops) => { | |
| return ops.filter((x) => DATE_OPERATIONS.has(x)).length > 0; | |
| }; | |
| export const hasDateOperation = (ops) => { | |
| return ops.some((x) => DATE_OPERATIONS.has(x)); | |
| }; |
| {isFacetCountEnabled === true ? ( | ||
| <Checkbox | ||
| disabled={isEditMode} | ||
| label={`${label} (${count})`} | ||
| radio={!isMulti} | ||
| checked={ | ||
| isMulti | ||
| ? !!facetValue?.find((f) => f.value === value) | ||
| : facetValue && facetValue.value === value | ||
| } | ||
| onChange={(e, { checked }) => | ||
| onChange( | ||
| facet.field.value, | ||
| isMulti | ||
| ? [ | ||
| ...facetValue | ||
| .filter((f) => f.value !== value) | ||
| .map((f) => f.value), | ||
| ...(checked ? [value] : []), | ||
| ] | ||
| : checked | ||
| ? value | ||
| : null, | ||
| ) | ||
| } | ||
| /> | ||
| ) : ( | ||
| <Checkbox | ||
| disabled={isEditMode} | ||
| label={label} | ||
| radio={!isMulti} | ||
| checked={ | ||
| isMulti | ||
| ? !!facetValue?.find((f) => f.value === value) | ||
| : facetValue && facetValue.value === value | ||
| } | ||
| onChange={(e, { checked }) => | ||
| onChange( | ||
| facet.field.value, | ||
| isMulti | ||
| ? [ | ||
| ...facetValue | ||
| .filter((f) => f.value !== value) | ||
| .map((f) => f.value), | ||
| ...(checked ? [value] : []), | ||
| ] | ||
| : checked | ||
| ? value | ||
| : null, | ||
| ) | ||
| } | ||
| /> | ||
| )} |
There was a problem hiding this comment.
style: The entire Checkbox component is duplicated in both branches of the conditional. Consider extracting the common Checkbox logic into a separate function or component to reduce code duplication and improve maintainability.
| {isFacetCountEnabled === true ? ( | |
| <Checkbox | |
| disabled={isEditMode} | |
| label={`${label} (${count})`} | |
| radio={!isMulti} | |
| checked={ | |
| isMulti | |
| ? !!facetValue?.find((f) => f.value === value) | |
| : facetValue && facetValue.value === value | |
| } | |
| onChange={(e, { checked }) => | |
| onChange( | |
| facet.field.value, | |
| isMulti | |
| ? [ | |
| ...facetValue | |
| .filter((f) => f.value !== value) | |
| .map((f) => f.value), | |
| ...(checked ? [value] : []), | |
| ] | |
| : checked | |
| ? value | |
| : null, | |
| ) | |
| } | |
| /> | |
| ) : ( | |
| <Checkbox | |
| disabled={isEditMode} | |
| label={label} | |
| radio={!isMulti} | |
| checked={ | |
| isMulti | |
| ? !!facetValue?.find((f) => f.value === value) | |
| : facetValue && facetValue.value === value | |
| } | |
| onChange={(e, { checked }) => | |
| onChange( | |
| facet.field.value, | |
| isMulti | |
| ? [ | |
| ...facetValue | |
| .filter((f) => f.value !== value) | |
| .map((f) => f.value), | |
| ...(checked ? [value] : []), | |
| ] | |
| : checked | |
| ? value | |
| : null, | |
| ) | |
| } | |
| /> | |
| )} | |
| <Checkbox | |
| disabled={isEditMode} | |
| label={isFacetCountEnabled === true ? `${label} (${count})` : label} | |
| radio={!isMulti} | |
| checked={ | |
| isMulti | |
| ? !!facetValue?.find((f) => f.value === value) | |
| : facetValue && facetValue.value === value | |
| } | |
| onChange={(e, { checked }) => | |
| onChange( | |
| facet.field.value, | |
| isMulti | |
| ? [ | |
| ...facetValue | |
| .filter((f) => f.value !== value) | |
| .map((f) => f.value), | |
| ...(checked ? [value] : []), | |
| ] | |
| : checked | |
| ? value | |
| : null, | |
| ) | |
| } | |
| /> |
| return variation; | ||
| }; | ||
|
|
||
| const isfunc = (obj) => isFunction(obj) || typeof obj === 'function'; |
There was a problem hiding this comment.
style: The isfunc helper function duplicates lodash's isFunction. Consider using only the imported isFunction from lodash for consistency.
| const isfunc = (obj) => isFunction(obj) || typeof obj === 'function'; | |
| const isfunc = (obj) => isFunction(obj); |
| Object.assign( | ||
| {}, | ||
| ...Object.keys(obj).map((k) => { | ||
| const reject = k !== 'properties' && !isfunc(obj[k]); |
There was a problem hiding this comment.
logic: Logic in _filtered appears inverted - reject variable name suggests filtering out, but the condition returns the property when reject is true
| ? deserializeQuery(JSON.stringify(data?.query?.query)) | ||
| : []; | ||
|
|
||
| let intializeFacetWithQueryValue = []; |
There was a problem hiding this comment.
syntax: Variable name has typo: 'intialize' should be 'initialize'
| let intializeFacetWithQueryValue = []; | |
| let initializeFacetWithQueryValue = []; |
|
Obsolete. Closing. |
No description provided.