Skip to content

Conversation

@AndreiSirbu
Copy link
Collaborator

In this commit I've updated the algolia search modal to include a product dropdown and I need to test it out on dev first.
Screenshot 2026-01-30 at 18 58 32

@github-actions
Copy link
Contributor

Pull Request Review: Algolia Search with Product Dropdown

Overview

This PR adds a product filter dropdown to the Algolia search modal, allowing users to filter search results by specific Netwrix products. The implementation involves swizzling Docusaurus's SearchBar component to add custom functionality.


Code Quality & Best Practices

✅ Strengths

  1. Good separation of concerns - ProductSelect component is well-isolated
  2. Proper React hooks usage - useCallback, useMemo, useState, useEffect are used appropriately
  3. Consistent with Docusaurus patterns - Follows the swizzling approach correctly
  4. LocalStorage persistence - Good UX decision to remember user's product filter preference

⚠️ Issues Found

1. Bug: Undefined observer in Root.js cleanup (Critical)

Location: src/theme/Root.js:29

The cleanup function references observer but it's commented out:

// const observer = new MutationObserver(updateFavicon);
// ...
return () => observer.disconnect();  // ❌ observer is undefined!

Impact: This will throw an error when the component unmounts.

Fix: Either uncomment the observer code or remove the cleanup:

return () => {}; // or just remove the return statement

2. Potential Race Condition in Search Refresh (Medium)

Location: src/theme/SearchBar/index.js:286-299

The product change handler uses setTimeout with 0ms delay to find and trigger the search input:

setTimeout(() => {
    const input = document.querySelector('.DocSearch-Input') || ...
    if (input) {
        input.value = value;
        input.dispatchEvent(new Event('input', {bubbles: true}));
    }
}, 0);

Issues:

  • No guarantee the input will exist after 0ms
  • Silent failure if input isn't found (user changes filter but sees no effect)
  • Relies on implementation details of DocSearch DOM structure

Recommendation:

  • Add error handling or user feedback if input isn't found
  • Consider using a retry mechanism with a timeout
  • Add a loading state while the search refreshes

3. Hard-coded Product List (Medium Priority)

Location: src/theme/SearchBar/index.js:127-147

The PRODUCT_OPTIONS array duplicates product information that exists in src/config/products.js.

Issues:

  • Maintenance burden: must update two places when adding/removing products
  • Risk of inconsistency between product names and Algolia facet values
  • The comment "Values MUST match Algolia facet values exactly" suggests fragility

Recommendation:

import { PRODUCTS } from '@site/src/config/products.js';

const PRODUCT_OPTIONS = [
    {label: 'All products', value: '__all__'},
    ...PRODUCTS.map(p => ({
        label: p.name,
        value: p.name // or p.algoliaFacetValue if added to config
    }))
];

4. Missing Accessibility Features (Medium Priority)

Location: src/theme/SearchBar/index.js:149-166

The ProductSelect has minimal accessibility:

  • ✅ Has aria-label
  • ❌ No visual focus indicator styling
  • ❌ No keyboard shortcut hints
  • ❌ Dropdown appears disconnected from search (no visual grouping)

Recommendation:

  • Add focus styles to the select element
  • Consider adding a label element for better screen reader support
  • Add visual grouping (border, background) to show it's part of search

Performance Considerations

✅ Good

  1. Memoization of facet filters - productFacetFilters is properly memoized
  2. Lazy loading of DocSearch modal - Modal is imported only when needed
  3. LocalStorage for persistence - Efficient way to remember user preference

⚠️ Potential Issues

Portal Re-rendering

Location: src/theme/SearchBar/index.js:335-350

The portal is re-created on every render when portalTarget exists. While React portals are efficient, the wrapper div is recreated each time.

Minor optimization:

const selectElement = useMemo(() => (
    <div style={{ /* ... */ }}>
        <ProductSelect value={product} onChange={onChangeProduct}/>
    </div>
), [product, onChangeProduct]);

return (
    <>
        <DocSearch ... />
        {portalTarget && createPortal(selectElement, portalTarget)}
    </>
);

Security Concerns

✅ No major security issues found

  1. LocalStorage usage is safe - Only storing user preference string
  2. No XSS risks - No dangerouslySetInnerHTML or unescaped user input
  3. No API keys exposed - Using existing Algolia config from docusaurus.config.js

💡 Minor Suggestions

  • Consider validating the localStorage value against known products before using it
  • Add error boundary in case DocSearch modal fails to load

Test Coverage

❌ No tests included

Recommendation:
This feature would benefit from tests covering:

  1. Product filter persistence (localStorage)
  2. Search refresh when filter changes
  3. Portal rendering when modal opens
  4. Fallback behavior when DocSearch fails to load

Example test cases:

describe('SearchBar with Product Filter', () => {
    it('should remember last selected product', () => { ... });
    it('should trigger search when product changes', () => { ... });
    it('should render select in modal header', () => { ... });
    it('should handle missing localStorage gracefully', () => { ... });
});

CSS & Styling

Issues in src/theme/SearchBar/styles.css

  1. Inline styles mixed with CSS file - ProductSelect uses inline styles instead of CSS classes
  2. Hardcoded z-index calculation - z-index: calc(var(--ifm-z-index-fixed) + 1) could conflict

Recommendation:
Move inline styles to the CSS file:

.DocSearch-ProductSelect {
  height: 36px;
  padding: 0 8px;
  border: 1px solid var(--docsearch-searchbox-border-color, #eee);
  border-radius: 4px;
  background: var(--docsearch-searchbox-background);
  color: var(--docsearch-text-color);
  font-size: 14px;
  cursor: pointer;
}

.DocSearch-ProductSelect:focus {
  outline: 2px solid var(--docsearch-primary-color);
  outline-offset: 2px;
}

.DocSearch-ProductSelect-Wrapper {
  display: flex;
  align-items: center;
  gap: 8px;
  margin-right: 8px;
}

Recommendations Summary

Must Fix Before Merge

  1. ✅ Fix undefined observer bug in Root.js:29
  2. ⚠️ Add error handling for search input not found

Should Fix Soon

  1. Consider importing products from central config
  2. Add accessibility improvements (focus styles, better labeling)
  3. Move inline styles to CSS file

Nice to Have

  1. Add tests for the new functionality
  2. Add loading/feedback when filter changes
  3. Optimize portal re-rendering

Testing Notes

Since this PR is marked as "testing the new algolia search", here are specific things to test:

  1. Filter Persistence

    • Select a product, refresh page - filter should be remembered
    • Clear localStorage - should default to "All products"
  2. Search Functionality

    • Search with "All products" - should return results from all products
    • Switch to specific product - results should filter immediately
    • Type new query after filter change - should respect current filter
  3. UI/UX

    • Dropdown should be visible and accessible
    • Should work on mobile viewport
    • Dark mode compatibility
    • Keyboard navigation (Tab, Arrow keys, Enter)
  4. Edge Cases

    • Very long product names
    • Product not in Algolia index
    • Slow network (modal loading)

Conclusion

This is a solid feature implementation that follows Docusaurus patterns well. The main concern is the critical bug in Root.js that will cause errors. Once that's fixed and some minor improvements are made, this will be a valuable addition to the search experience.

Recommendation: Fix the critical bug, then merge for testing on dev. Address other issues based on dev testing feedback.

@AndreiSirbu AndreiSirbu marked this pull request as draft February 2, 2026 16:23
@hilram7 hilram7 assigned hilram7 and unassigned hilram7 Feb 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants