[WEB-4885] feat: new filters architecture and UI components#7802
[WEB-4885] feat: new filters architecture and UI components#7802sriramveeraghanta merged 8 commits intopreviewfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a rich-filters subsystem (types, utils, factories, operations, validators, UI) and a shared-state rich-filters store; extends dropdowns (defaultOpen, portal rendering) and UI helpers; reorganizes many type/constant barrels and adds related package dependencies and exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as AddFilterButton / FilterItem
participant CM as FilterConfigManager
participant FI as FilterInstance
participant Ops as operators (utils)
participant Ext as External consumer
UI->>CM: read allAvailableConfigs
UI->>UI: render add-filter options
UI-->>UI: user selects property
UI->>CM: getConfigByProperty(property)
CM-->>UI: config (with firstOperator)
UI->>Ops: getOperatorForPayload(firstOperator)
Ops-->>UI: { operator, isNegation }
UI->>FI: addCondition(LOGICAL_OPERATOR.AND, payload, isNegation)
FI->>FI: sanitize/stabilize expression (utils/operations)
FI->>Ext: onExpressionChange(externalExpression)
sequenceDiagram
autonumber
participant DateUI as DateRangeDropdown
participant Portal as document.body
participant Popper as usePopper
DateUI->>DateUI: mount (defaultOpen?)
alt renderInPortal true
DateUI->>Portal: createPortal(Options)
else
DateUI->>DateUI: render Options inline
end
DateUI->>Popper: compute popover position
DateUI-->>User: user selects range -> onSelect -> update condition
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/utils/src/datetime.ts (1)
583-590: isValidDate() is incorrect for Date inputs and misaligned with its type guard.It returns true only for strings; valid Date objects return false. Leverage date‑fns isValid and parseISO.
-export const isValidDate = (date: unknown): date is string | Date => - typeof date === "string" && !isNaN(Date.parse(date)); +export const isValidDate = (date: unknown): date is string | Date => { + if (date instanceof Date) return isValid(date); + if (typeof date === "string") { + const parsed = parseISO(date); + return isValid(parsed); + } + return false; +}Optional: consider a less confusing name (e.g., isParsableDate) to avoid ambiguity with date‑fns isValid in call sites.
apps/web/package.json (1)
80-80: Align @types/uuid to workspace catalog (use 9.0.8)pnpm-lock.yaml shows both @types/uuid@8.3.4 and @types/uuid@9.0.8; update apps/web/package.json (line 80) from "^8.3.4" to the workspace-pinned 9.0.8 (or remove if the types are unnecessary) to avoid duplicate typings.
packages/constants/src/rich-filters/operator-labels/extended.ts (1)
1-22: Empty constant maps need to be populated.All four operator label maps are initialized as empty objects. This appears to be incomplete implementation, as the maps are typed to hold operator labels but contain no actual mappings. The empty
Record<never, string>types for negated maps are also suspicious.These maps should contain the actual label mappings for extended operators. For example:
-export const EXTENDED_OPERATOR_LABELS_MAP: Record<TExtendedSupportedOperators, string> = {} as const; +export const EXTENDED_OPERATOR_LABELS_MAP: Record<TExtendedSupportedOperators, string> = { + // Add actual operator mappings here + // Example: [SOME_OPERATOR]: "Operator Label", +} as const;The
Record<never, string>type for negated maps indicates they can never have any properties, which seems incorrect if these are meant to be populated later.
🧹 Nitpick comments (86)
packages/utils/src/datetime.ts (1)
569-569: Unit copy uses "hr" for all counts. Confirm consistency.Elsewhere you use "min"/"sec". If product copy expects "hrs" for plurals, adjust here.
packages/types/src/utils.ts (1)
9-9: Useful alias; semantics LGTM.
SingleOrArray<T>correctly preservesnull | undefinedand widens others toT | T[].Consider adding a brief doc comment and/or alias
MaybeArray<T>for discoverability.packages/ui/src/dropdowns/helper.tsx (1)
1-1: Remove or action the FIXME.Top-level FIXME without context creates noise; either replace with a specific TODO/issue link or drop it.
-// FIXME: fix this!!! +// TODO(UX): track dropdown helper cleanup in issue WEB-XXXX if needed.packages/constants/src/rich-filters/option.ts (1)
60-63: Consider optionalconfigand stronger typing.If
configis optional at call-sites, make it optional; also allow narrowing via a generic when needed.-export type TFilterOptions<E extends TExternalFilter> = { - expression: Partial<TExpressionOptions<E>>; - config: Partial<TConfigOptions>; -}; +export type TFilterOptions< + E extends TExternalFilter, + C extends TConfigOptions = TConfigOptions +> = { + expression: Partial<TExpressionOptions<E>>; + config?: Partial<C>; +};packages/ui/src/dropdowns/custom-search-select.tsx (4)
160-166: Clamp “full” height to the viewport (not parent).In a portal,
max-h-fullcan exceed the viewport if no ancestor has an explicit height. Prefer viewport‑based caps.Apply this diff:
- "max-h-full": maxHeight === "full", + // Use viewport-based cap to avoid overflow in portals + "max-h-screen max-h-[100dvh]": maxHeight === "full",Confirm Tailwind arbitrary values are enabled; otherwise use only
max-h-screen.
54-61: Close on selection for keyboard users (single‑select).
onClickon Option won’t fire for keyboard selection; close viaonChangewhen!multiple.Apply this diff (moves comboboxProps after
closeDropdownand wrapsonChange):- const comboboxProps: any = { - value, - onChange, - disabled, - }; - - if (multiple) comboboxProps.multiple = true; + // after closeDropdown() + const handleChange = (val: any) => { + onChange?.(val); + if (!multiple) closeDropdown(); + }; + const comboboxProps: any = { + value, + onChange: handleChange, + disabled, + ...(multiple ? { multiple: true } : {}), + };Also applies to: 68-71
90-93: Avoid duplicate onOpen calls.You call
onOpeninsideopenDropdown()and also whenComboboxrender‑propopenflips. This can double‑fire.Apply this diff to rely on the controlled
isOpenpath:- {({ open }: { open: boolean }) => { - if (open && onOpen) onOpen(); + {() => {
141-144: Prevent nested scrollbars.Outer container already wraps a scrollable list;
overflow-y-scrollhere causes double scrollbars.Apply this diff:
- className={cn( - "my-1 overflow-y-scroll rounded-md border-[0.5px] border-custom-border-300 bg-custom-background-100 px-2 py-2.5 text-xs shadow-custom-shadow-rg focus:outline-none min-w-48 whitespace-nowrap z-30", + className={cn( + "my-1 overflow-hidden rounded-md border-[0.5px] border-custom-border-300 bg-custom-background-100 px-2 py-2.5 text-xs shadow-custom-shadow-rg focus:outline-none min-w-48 whitespace-nowrap z-30",apps/web/core/components/dropdowns/date.tsx (2)
45-46: Optional: focus the trigger when opened by default.Call onOpen after mount if defaultOpen to keep behavior consistent with manual open.
React.useEffect(() => { if (defaultOpen) onOpen?.(); // eslint-disable-next-line react-hooks/exhaustive-deps }, [defaultOpen]);
45-46: Prop is “default” only; changes after mount are ignored.If you expect defaultOpen to react to prop changes, sync it into isOpen via an effect (as above) or document that it’s initial‑only.
Also applies to: 65-65
packages/utils/src/rich-filters/operations/transformation/shared.ts (1)
5-11: Docstring claims OR/NOT support that doesn’t exist in typesCurrent types limit TFilterGroupNode to AND groups and processGroupNode only handles onAndGroup. Please align the doc to avoid confusion.
Apply:
- * Handles AND/OR groups with children and NOT groups with single child. + * Handles AND groups by transforming their children.packages/utils/src/rich-filters/validators/shared.ts (1)
12-22: Unused parameter and misleading docs; simplify to the actual behavior_preserveNotGroups is unused and the code simply unwraps when there is exactly one child. Either implement the param or remove it and tighten the docs.
Apply:
-export const shouldUnwrapGroup = <P extends TFilterProperty>(group: TFilterGroupNode<P>, _preserveNotGroups = true) => { +export const shouldUnwrapGroup = <P extends TFilterProperty>(group: TFilterGroupNode<P>): boolean => { const children = getGroupChildren(group); @@ - // Unwrap AND/OR groups with single children, and NOT groups if preserveNotGroups is false + // Unwrap groups with a single child return true; };And update the docstring accordingly (AND-only).
packages/types/src/rich-filters/field-types/extended.ts (2)
1-1: Use type‑only import to avoid accidental value importMinor hygiene tweak.
-import { TFilterValue } from "../expression"; +import type { TFilterValue } from "../expression";
6-13: Placeholder surface is fine; consider a short rationale commentUsing an empty const and never config is an acceptable placeholder. Add a note so future contributors understand why it’s empty.
-export const EXTENDED_FILTER_FIELD_TYPE = {} as const; +// Intentionally empty: extended field types are wired by consumers or future releases. +export const EXTENDED_FILTER_FIELD_TYPE = {} as const;packages/types/src/rich-filters/operator-configs/extended.ts (1)
1-14: Use type-only import and documentunknownintentSwitch the import to a type-only import and add a short comment explaining that
unknownis intentional soCore & Extendedpreserves Core keys. ConfirmedTOperatorSpecificConfigsuses intersection (&) in packages/types/src/rich-filters/operator-configs/index.ts.-import { TFilterValue } from "../expression"; +import type { TFilterValue } from "../expression"; @@ -// ----------------------------- Extended Operator Specific Configs ----------------------------- -export type TExtendedOperatorSpecificConfigs<_V extends TFilterValue> = unknown; +// ----------------------------- Extended Operator Specific Configs ----------------------------- +// Keep as `unknown` so that `Core & Extended` preserves Core keys until Extended is defined. +export type TExtendedOperatorSpecificConfigs<_V extends TFilterValue> = unknown;packages/utils/src/rich-filters/operations/traversal/shared.ts (1)
9-23: Docstring promises NOT-group handling that isn’t implemented.Either implement NOT-context inversion or clarify the JSDoc to avoid misleading callers. For now, it’s a pass-through which can surface incorrect operators in display when a condition is under a NOT group.
Apply this doc tweak until negation is implemented:
- * Helper function to get the display operator for a condition. - * This checks for NOT group context and applies negation if needed. + * Helper function to get the display operator for a condition. + * NOTE: Placeholder — currently returns operator as-is. TODO: Apply NOT-group negation when present.packages/shared-state/src/utils/rich-filter.helper.ts (1)
35-44: Operator negation is hardcoded off.getOperatorForPayload currently returns isNegation = false, so negated conditions passed into this helper will be lost. If negation isn’t supported yet, gate the feature and/or document it on the builder types to avoid silent drops.
packages/shared-state/src/store/rich-filters/adapter.ts (1)
4-10: Doc claims “Provides common utilities” but class exposes no utilities.Tighten the comment to match reality or add protected helpers in a follow-up.
- * Provides common utilities for creating and manipulating filter nodes. + * Base contract for converting between external filter formats and internal expressions.packages/types/src/rich-filters/field-types/index.ts (1)
19-21: Optional: export the key union for ergonomics.Handy when switching on keys without importing the constant.
+export type TFilterFieldTypeKey = keyof typeof FILTER_FIELD_TYPE;apps/web/core/components/dropdowns/date-range.tsx (2)
99-104: defaultOpen only seeds state; no focus sync.If you expect focus on mount when defaultOpen = true, onOpen won’t run. Consider a mount-effect that calls onOpen when defaultOpen is true.
+ useEffect(() => { + if (defaultOpen) onOpen(); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []);
269-278: Consider closing on complete selection (optional).Add a closeOnSelect?: boolean (default false) to align with common datepickers; when true and range is complete, setIsOpen(false).
- onSelect={(val: DateRange | undefined) => { - onSelect?.(val); - }} + onSelect={(val: DateRange | undefined) => { + onSelect?.(val); + if (val?.from && val?.to && closeOnSelect) setIsOpen(false); + }}apps/web/core/components/rich-filters/filter-value-input/root.tsx (3)
50-55: Type cast mismatch for condition in MULTI_SELECT branch.Use the same “ForDisplay” condition type as other branches; current cast drops display-operator typing.
Apply this diff:
- <MultiSelectFilterValueInput<P> - config={filterFieldConfig as TMultiSelectFilterFieldConfig<string>} - condition={condition as TFilterConditionNode<P, string>} + <MultiSelectFilterValueInput<P> + config={filterFieldConfig as TMultiSelectFilterFieldConfig<string>} + condition={condition as TFilterConditionNodeForDisplay<P, string>} isDisabled={isDisabled} onChange={(value) => onChange(value as SingleOrArray<V>)} />
35-45: Remove unsafe casts via typed type guards.Define field-config type guards so each branch narrows without assertions. This also keeps V aligned with value kinds.
Additional code (new helpers; place in a shared types/util file):
export const isSingleSelect = <V extends TFilterValue>( c: TSupportedFilterFieldConfigs<V> ): c is TSingleSelectFilterFieldConfig<string> => c?.type === FILTER_FIELD_TYPE.SINGLE_SELECT; export const isMultiSelect = <V extends TFilterValue>( c: TSupportedFilterFieldConfigs<V> ): c is TMultiSelectFilterFieldConfig<string> => c?.type === FILTER_FIELD_TYPE.MULTI_SELECT; export const isDate = <V extends TFilterValue>( c: TSupportedFilterFieldConfigs<V> ): c is TDateFilterFieldConfig<string> => c?.type === FILTER_FIELD_TYPE.DATE; export const isDateRange = <V extends TFilterValue>( c: TSupportedFilterFieldConfigs<V> ): c is TDateRangeFilterFieldConfig<string> => c?.type === FILTER_FIELD_TYPE.DATE_RANGE;Then:
if (isSingleSelect(filterFieldConfig)) { /* no casts */ } if (isMultiSelect(filterFieldConfig)) { /* no casts */ } if (isDate(filterFieldConfig)) { /* no casts */ } if (isDateRange(filterFieldConfig)) { /* no casts */ }
83-88: Fallback: add a11y hint.Consider role="status" and aria-disabled to clarify non-interactive state.
- <div className="h-full flex items-center px-4 text-xs text-custom-text-400 transition-opacity duration-200 cursor-not-allowed"> + <div role="status" aria-disabled className="h-full flex items-center px-4 text-xs text-custom-text-400 transition-opacity duration-200 cursor-not-allowed">packages/constants/src/rich-filters/operator-labels/index.ts (1)
18-23: Provide a safe accessor and trim redundant typing.
- Add getOperatorLabel/getDateOperatorLabel with EMPTY_OPERATOR_LABEL fallback.
- Optional: either keep explicit Record<…> or drop the trailing “as const” (both is redundant).
Additional code (new exports):
export const getOperatorLabel = (op: TAllAvailableOperatorsForDisplay) => OPERATOR_LABELS_MAP[op] ?? EMPTY_OPERATOR_LABEL; export const getDateOperatorLabel = (op: TAllAvailableDateFilterOperatorsForDisplay) => DATE_OPERATOR_LABELS_MAP[op] ?? EMPTY_OPERATOR_LABEL;Also applies to: 27-31
apps/web/core/components/rich-filters/filter-value-input/date/range.tsx (1)
47-55: Optional UX: indicate invalid state to screen readers.If DateRangeDropdown supports it, pass aria-invalid/isInvalid when isIncomplete is true.
packages/utils/src/rich-filters/validators/core.ts (1)
33-52: Consider adding a default case for unknown node types.While the current implementation handles condition and group nodes, it silently returns
falsefor any unknown node types. Consider adding explicit handling or logging for unexpected node types.export const shouldNotifyChangeForExpression = <P extends TFilterProperty>( expression: TFilterExpression<P> | null ): boolean => { if (!expression) { return false; } // If it's a condition, check its value if (isConditionNode(expression)) { return hasValidValue(expression.value); } // If it's a group, check if any of its children have meaningful values if (isGroupNode(expression)) { const children = getGroupChildren(expression); return children.some((child) => shouldNotifyChangeForExpression(child)); } + // Log unexpected node types in development + if (process.env.NODE_ENV === 'development') { + console.warn('Unexpected filter expression node type:', expression); + } return false; };apps/web/core/components/rich-filters/filters-row.tsx (3)
54-58: Consider extracting magic number to a constant.The 240ms timeout value should be extracted to a named constant for better maintainability and clarity about its purpose.
+const UPDATE_CONFIRMATION_DELAY_MS = 240; // Prevents UI flickering during update + const handleUpdate = useCallback(async () => { setIsUpdating(true); await filter.updateView(); - setTimeout(() => setIsUpdating(false), 240); // To avoid flickering + setTimeout(() => setIsUpdating(false), UPDATE_CONFIRMATION_DELAY_MS); }, [filter]);
83-83: Undefined constantCOMMON_VISIBILITY_BUTTON_CLASSNAME.The constant
COMMON_VISIBILITY_BUTTON_CLASSNAMEis used here but defined later at line 165. This works due to JavaScript hoisting but violates the principle of defining before use.Move the constant definitions before the component:
+const COMMON_VISIBILITY_BUTTON_CLASSNAME = "py-0.5 px-2 text-custom-text-300 hover:text-custom-text-100 rounded-full"; +const COMMON_OPERATION_BUTTON_CLASSNAME = "py-1"; + export const FiltersRow = observer( <K extends TFilterProperty, E extends TExternalFilter>(props: TFiltersRowProps<K, E>) => {
168-185: Consider memoizing ElementTransition component.The
ElementTransitioncomponent could benefit from memoization since it only depends on its props and doesn't need to re-render when parent state changes.-const ElementTransition = observer((props: TElementTransitionProps) => ( +const ElementTransition = React.memo(observer((props: TElementTransitionProps) => ( <Transition show={props.show} enter="transition ease-out duration-200" enterFrom="opacity-0 scale-95" enterTo="opacity-100 scale-100" leave="transition ease-in duration-150" leaveFrom="opacity-100 scale-100" leaveTo="opacity-0 scale-95" > {props.children} </Transition> -)); +)));apps/web/core/components/rich-filters/filter-value-input/select/single.tsx (2)
31-33: Consider handling config changes more robustly.The effect reloads options whenever config changes, but doesn't handle the case where options might already be an array (not a function). Also consider clearing options when config is undefined.
useEffect(() => { + // If config has static options (array), set them directly + if ("getOptions" in config && Array.isArray(config.getOptions)) { + setOptions(config.getOptions); + return; + } + + // Otherwise load async options loadOptions({ config, setOptions, setLoading }); }, [config]);
54-54: Verify defaultOpen behavior — avoid auto-opening every empty selectfilters-row -> FilterItem -> FilterValueInput -> SingleSelectFilterValueInput; defaultOpen={!condition.value} will open every empty select when multiple filters render. Remove defaultOpen or gate it behind an explicit autoOpen prop (only true for newly-added conditions) to prevent multiple dropdowns opening at once. Files: apps/web/core/components/rich-filters/filter-value-input/select/single.tsx and the filters renderer (apps/web/core/components/rich-filters/filters-row.tsx / filter-item.tsx).
apps/web/core/components/rich-filters/filter-item.tsx (4)
66-67: Avoid using void as an expression statement.The void operator is being used to force a reflow, but this is unconventional and the comment doesn't align with the actual mechanism.
Apply this diff to use a more conventional approach:
- // Force a reflow to ensure the initial state is applied - void element.offsetWidth; + // Force a reflow to ensure the initial state is applied + element.offsetWidth; // eslint-disable-line @typescript-eslint/no-unused-expressions
75-77: Cleanup function modifies DOM state unexpectedly.The cleanup function resets opacity and transform when the component unmounts or dependencies change, which could cause unwanted visual artifacts if the component is being removed from the DOM anyway.
Consider removing the cleanup function or only applying it when the component remains mounted:
- return () => { - applyInitialStyles(); - }; + // No cleanup needed when unmounting
131-134: Consider semantic HTML for better accessibility.Using a div with aria-disabled instead of a button element reduces accessibility for screen reader users.
The CustomSearchSelect's customButton should ideally render a semantic button element that properly conveys its interactive nature and disabled state to assistive technologies. Consider passing a button element or ensuring CustomSearchSelect handles the disabled state properly.
153-153: Consider a more specific aria-label.The aria-label could be more descriptive by including the filter property being removed.
- aria-label="Remove filter" + aria-label={`Remove ${filterConfig?.label || 'filter'}`}packages/utils/src/rich-filters/types/core.ts (2)
45-50: Redundant type guard implementation.The
hasChildrenPropertyfunction essentially duplicates the logic thatisAndGroupNodealready provides, since AND groups are defined to have a children property.Consider removing this function or documenting why it's needed separately from
isAndGroupNodeif there's a specific use case.
48-49: Unnecessary type assertion.The type assertion to
{ children?: unknown }is redundant since you're already checking the property exists on the original group parameter.- const groupWithChildren = group as { children?: unknown }; - return "children" in group && Array.isArray(groupWithChildren.children); + return "children" in group && Array.isArray(group.children);packages/utils/src/rich-filters/types/shared.ts (2)
28-28: Error message could be more helpful.The error message doesn't properly stringify the group object, which would make debugging difficult.
- throw new Error(`Invalid group node: unknown logical operator ${group}`); + throw new Error(`Invalid group node: unknown logical operator ${JSON.stringify(group.logicalOperator)}`);
32-32: Misleading comment about OR/NOT groups.The comment mentions handling "AND/OR groups (children array) and NOT groups (single child)", but the implementation only handles AND groups.
Update the comment to accurately reflect the current implementation:
-/** - * Gets the children of a group node, handling AND/OR groups (children array) and NOT groups (single child). - * Uses processGroupNode for consistent group type handling. - * @param group - The group node to get children from - * @returns Array of child expressions - */ +/** + * Gets the children of a group node. Currently only supports AND groups. + * Uses processGroupNode for consistent group type handling. + * @param group - The group node to get children from + * @returns Array of child expressions + */packages/types/src/rich-filters/operator-configs/index.ts (1)
48-51: Consider documenting Map type usage.Using a Map type in TypeScript type definitions is less common and might benefit from additional documentation about its intended usage pattern.
Consider adding a comment explaining when and why a Map is preferred over a regular object type here, or consider using a more conventional object type if the Map semantics aren't specifically required:
// Alternative object-based approach: export type TOperatorConfigRecord<V extends TFilterValue> = { [K in keyof TOperatorSpecificConfigs<V>]: TOperatorSpecificConfigs<V>[K]; };apps/web/core/components/rich-filters/filter-value-input/select/multi.tsx (2)
33-35: Add proper cleanup and dependency tracking for loadOptions effect.The effect doesn't include
configin its dependency array, which could lead to stale closures or missed updates when config changes. Additionally, consider handling cleanup for any pending async operations.Apply this diff to fix the effect dependencies:
useEffect(() => { loadOptions({ config, setOptions, setLoading }); -}, [config]); +}, [config]);Actually, the dependency is already correct. However, consider adding cleanup for pending async operations:
useEffect(() => { + let isMounted = true; + const loadOptionsWithCleanup = async () => { + if ("getOptions" in config && typeof config.getOptions === "function") { + setLoading(true); + try { + const result = await config.getOptions(); + if (isMounted) { + setOptions(result); + } + } catch (error) { + if (isMounted) { + console.error("Failed to load options:", error); + } + } finally { + if (isMounted) { + setLoading(false); + } + } + } + }; + loadOptionsWithCleanup(); + return () => { + isMounted = false; + }; - loadOptions({ config, setOptions, setLoading }); }, [config]);
37-39: Consider memoizing the handleSelectChange callback.The
handleSelectChangefunction is recreated on every render, which could potentially cause unnecessary re-renders of child components.Apply this diff to optimize the callback:
+import React, { useState, useEffect, useMemo, useCallback } from "react"; -const handleSelectChange = (values: string[]) => { - onChange(values); -}; +const handleSelectChange = useCallback((values: string[]) => { + onChange(values); +}, [onChange]);packages/utils/src/rich-filters/operators/core.ts (1)
1-1: Consider using direct property access instead of lodash get.Since you're accessing simple object properties with known keys, using lodash's
getfunction adds unnecessary overhead and an additional dependency.Apply this diff to simplify the implementation:
-import get from "lodash/get"; // plane imports import { DATE_OPERATOR_LABELS_MAP, EMPTY_OPERATOR_LABEL, OPERATOR_LABELS_MAP } from "@plane/constants";Then update the functions:
export const getOperatorLabel = (operator: TAllAvailableOperatorsForDisplay | undefined): string => { if (!operator) return EMPTY_OPERATOR_LABEL; - return get(OPERATOR_LABELS_MAP, operator, EMPTY_OPERATOR_LABEL); + return OPERATOR_LABELS_MAP[operator] ?? EMPTY_OPERATOR_LABEL; }; export const getDateOperatorLabel = (operator: TAllAvailableDateFilterOperatorsForDisplay | undefined): string => { if (!operator) return EMPTY_OPERATOR_LABEL; - return get(DATE_OPERATOR_LABELS_MAP, operator, EMPTY_OPERATOR_LABEL); + return DATE_OPERATOR_LABELS_MAP[operator] ?? EMPTY_OPERATOR_LABEL; };apps/web/core/components/rich-filters/filter-value-input/date/single.tsx (1)
22-44: Consider memoizing the onChange callback to prevent unnecessary re-renders.The inline onChange handler creates a new function on every render, which could cause the DateDropdown to re-render unnecessarily.
Apply this diff to optimize the component:
+import React, { useCallback } from "react"; export const SingleDateFilterValueInput = observer( <P extends TFilterProperty>(props: TSingleDateFilterValueInputProps<P>) => { const { config, condition, isDisabled, onChange } = props; // derived values const conditionValue = typeof condition.value === "string" ? condition.value : null; + const handleDateChange = useCallback((value: Date | null) => { + const formattedDate = value ? renderFormattedPayloadDate(value) : null; + onChange(formattedDate); + }, [onChange]); return ( <DateDropdown value={conditionValue} - onChange={(value: Date | null) => { - const formattedDate = value ? renderFormattedPayloadDate(value) : null; - onChange(formattedDate); - }} + onChange={handleDateChange} buttonClassName={cn("rounded-none", {apps/web/core/components/rich-filters/filter-value-input/select/selected-options-display.tsx (2)
36-46: Use option.id as key instead of array index.Using array indices as React keys can lead to rendering issues when the list order changes. Since options have unique IDs, use them instead.
Apply this diff to use stable keys:
-{selectedOptions.slice(0, displayCount).map((option, index) => ( - <React.Fragment key={index}> +{selectedOptions.slice(0, displayCount).map((option, index) => ( + <React.Fragment key={option.id}>
48-58: Transition component's show prop is redundant.The
showprop is hardcoded totruebut the component is already conditionally rendered withremainingCount > 0. Consider removing the redundant prop.Apply this diff to simplify:
{remainingCount > 0 && ( <Transition - show appear enter="transition-opacity duration-300" enterFrom="opacity-0" enterTo="opacity-100" className="text-custom-text-300 whitespace-nowrap ml-1" > +{remainingCount} more </Transition> )}apps/web/core/components/rich-filters/add-filters-button.tsx (1)
39-50: Consider memoizing filterOptions to avoid recalculation on every render.The transformation of
allAvailableConfigsto options format happens on every render. Since this depends only onfilter.configManager.allAvailableConfigs, it could be memoized.Apply this diff to optimize:
+import React, { useMemo } from "react"; export const AddFilterButton = observer( <P extends TFilterProperty, E extends TExternalFilter>(props: TAddFilterButtonProps<P, E>) => { // ... other code ... - // Transform available filter configs to CustomSearchSelect options format - const filterOptions = filter.configManager.allAvailableConfigs.map((config) => ({ - value: config.id, - content: ( - <div className="flex items-center gap-2 text-custom-text-200 transition-all duration-200 ease-in-out"> - {config.icon && ( - <config.icon className="size-4 text-custom-text-300 transition-transform duration-200 ease-in-out" /> - )} - <span>{config.label}</span> - </div> - ), - query: config.label.toLowerCase(), - })); + // Transform available filter configs to CustomSearchSelect options format + const filterOptions = useMemo(() => + filter.configManager.allAvailableConfigs.map((config) => ({ + value: config.id, + content: ( + <div className="flex items-center gap-2 text-custom-text-200 transition-all duration-200 ease-in-out"> + {config.icon && ( + <config.icon className="size-4 text-custom-text-300 transition-transform duration-200 ease-in-out" /> + )} + <span>{config.label}</span> + </div> + ), + query: config.label.toLowerCase(), + })), [filter.configManager.allAvailableConfigs]);packages/utils/src/rich-filters/operations/manipulation/core.ts (3)
35-38: Avoid in-place mutation of AND-group childrenPushing into children mutates the existing node, which can break memoization/identity checks in consumers. Return a new group with updated children.
- if (isGroupNode(expression) && isAndGroupNode(expression)) { - expression.children.push(condition); - return expression; - } + if (isGroupNode(expression) && isAndGroupNode(expression)) { + // avoid in-place mutation to preserve referential stability + return { ...expression, children: [...getGroupChildren(expression), condition] }; + }
61-71: Consider propagating shouldNotify intent (or remove it here)You always return shouldNotify: false. If the transform pipeline uses this flag for downstream updates, make it a parameter or set true when a replacement occurs. Otherwise, remove the flag from this call-site to avoid confusion.
111-120: Annotate explicit return typeMake the return type clear for callers.
-export const unwrapGroupIfNeeded = <P extends TFilterProperty>( +export const unwrapGroupIfNeeded = <P extends TFilterProperty>( group: TFilterGroupNode<P>, preserveNotGroups = true -) => { +): TFilterExpression<P> => {apps/web/core/components/rich-filters/filter-value-input/select/shared.tsx (2)
13-28: Ensure deterministic state on load failure (and consider cancellation)On error, options remain stale. Set an empty options list to keep UI consistent. Also consider a simple stale‑request guard or AbortController to avoid setting state after unmount.
try { const result = await config.getOptions(); setOptions(result); } catch (error) { console.error("Failed to load options:", error); + // reset options on failure + setOptions([]); } finally { setLoading?.(false); }
30-44: Minor: simplify query derivationlabel is already a string; toString() is redundant.
- query: option.label.toString().toLowerCase(), + query: option.label.toLowerCase(),packages/types/src/rich-filters/operators/core.ts (1)
31-39: Export individual operator types (and logical) for completenessDownstream code may need specific operator categories. Export these types and include logical as well.
-type TCoreEqualityOperator = (typeof CORE_EQUALITY_OPERATOR)[keyof typeof CORE_EQUALITY_OPERATOR]; -type TCoreCollectionOperator = (typeof CORE_COLLECTION_OPERATOR)[keyof typeof CORE_COLLECTION_OPERATOR]; -type TCoreComparisonOperator = (typeof CORE_COMPARISON_OPERATOR)[keyof typeof CORE_COMPARISON_OPERATOR]; +export type TCoreLogicalOperator = (typeof CORE_LOGICAL_OPERATOR)[keyof typeof CORE_LOGICAL_OPERATOR]; +export type TCoreEqualityOperator = (typeof CORE_EQUALITY_OPERATOR)[keyof typeof CORE_EQUALITY_OPERATOR]; +export type TCoreCollectionOperator = (typeof CORE_COLLECTION_OPERATOR)[keyof typeof CORE_COLLECTION_OPERATOR]; +export type TCoreComparisonOperator = (typeof CORE_COMPARISON_OPERATOR)[keyof typeof CORE_COMPARISON_OPERATOR]; /** * All core operators that can be used in filter conditions */ export type TCoreSupportedOperators = TCoreEqualityOperator | TCoreCollectionOperator | TCoreComparisonOperator;packages/types/src/rich-filters/operators/index.ts (1)
40-43: Consider extracting operator-related types to a dedicated types file.The operator type definitions are cleanly extracted from the const objects, providing good type safety. However, as this architecture grows, you might want to separate type definitions from the operator constant definitions for better organization.
packages/shared-state/src/store/rich-filters/config.ts (3)
150-156: Fix potential type safety issue ingetDisplayOperatorByValue.The type assertion
as typeof operatoron Line 153 could be unsafe. WhenoperatorConfig.singleValueOperatoris returned, its type might not match the generic parameterTpassed to the function.Consider adding a type guard or validation:
getDisplayOperatorByValue: IFilterConfig<P, V>["getDisplayOperatorByValue"] = computedFn((operator, value) => { const operatorConfig = this.getOperatorConfig(operator); if (operatorConfig?.type === FILTER_FIELD_TYPE.MULTI_SELECT && (Array.isArray(value) ? value.length : 0) <= 1) { - return operatorConfig.singleValueOperator as typeof operator; + // Ensure the singleValueOperator is compatible with the expected return type + const singleOp = operatorConfig.singleValueOperator; + return singleOp as T; } return operator; });
195-204: ReplacehasOwnPropertywithObject.hasOwnfor better safety.The use of
hasOwnPropertyon Line 198 could be problematic if the object has a customhasOwnPropertymethod. UseObject.hasOwnfor better safety.mutate: IFilterConfig<P, V>["mutate"] = action((updates) => { runInAction(() => { for (const key in updates) { - if (updates.hasOwnProperty(key)) { + if (Object.hasOwn(updates, key)) { const configKey = key as keyof TFilterConfig<P, V>; set(this, configKey, updates[configKey]); } } }); });
208-211: Document the purpose of_getAdditionalOperatorOptionshook.The private
_getAdditionalOperatorOptionsmethod appears to be a hook for subclasses but currently returns undefined. Add documentation explaining its intended use case.+ /** + * Hook method for subclasses to provide additional operator options. + * Override this method to add custom operator variants (e.g., negation options). + * @param operator - The base operator + * @param value - The current filter value + * @returns Additional operator option or undefined + */ private _getAdditionalOperatorOptions = ( _operator: TSupportedOperators, _value: V ): TOperatorOptionForDisplay | undefined => undefined;packages/types/src/rich-filters/field-types/core.ts (2)
17-20: ExtractTBaseDateFilterFieldConfigto shared types.
TBaseDateFilterFieldConfigis defined as a local type but might be useful in other contexts. Consider exporting it for reusability.-type TBaseDateFilterFieldConfig = TBaseFilterFieldConfig & { +export type TBaseDateFilterFieldConfig = TBaseFilterFieldConfig & { min?: Date; max?: Date; };
51-55: Document async behavior forgetOptionsfunction.The
getOptionsproperty can be either a static array or an async function. Document the expected behavior when the promise is rejected and how loading states should be handled in the UI.Also applies to: 63-68
packages/utils/src/rich-filters/factories/configs/shared.ts (2)
25-35: Type assertion increateFilterFieldConfigmay hide type errors.The function uses a type assertion (
as TSupportedFilterFieldConfigs<V>) which bypasses TypeScript's type checking. This could hide type mismatches at compile time.Consider using a type guard or validation function instead:
-): TSupportedFilterFieldConfigs<V> => config as TSupportedFilterFieldConfigs<V>; +): TSupportedFilterFieldConfigs<V> => { + // Runtime validation could be added here if needed + return config as TSupportedFilterFieldConfigs<V>; +};
50-53: Generic constraint onIFilterIconConfigis overly broad.The generic constraint
T extends string | number | boolean | object | undefinedincludesobjectwhich might be too permissive. Consider using a more specific constraint that matches the actual icon value types used in the application.-export interface IFilterIconConfig<T extends string | number | boolean | object | undefined = undefined> { +export interface IFilterIconConfig<T extends string | number | boolean | Date | undefined = undefined> { filterIcon?: React.FC<React.SVGAttributes<SVGElement>>; getOptionIcon?: (value: T) => React.ReactNode; }packages/shared-state/src/store/rich-filters/filter.ts (3)
345-377: Add null check for config retrieval inupdateConditionOperator.The method retrieves configs using optional chaining but doesn't handle the case where the config might be null before calling
getOperatorConfig.updateConditionOperator: IFilterInstance<P, E>["updateConditionOperator"] = action( (conditionId: string, operator: TSupportedOperators, isNegation: boolean) => { if (!this.expression) return; const conditionBeforeUpdate = cloneDeep(findNodeById(this.expression, conditionId)); if (!conditionBeforeUpdate || conditionBeforeUpdate.type !== FILTER_NODE_TYPE.CONDITION) return; + const propertyConfig = this.configManager.getConfigByProperty(conditionBeforeUpdate.property); + if (!propertyConfig) { + console.warn(`No config found for property: ${conditionBeforeUpdate.property}`); + return; + } + // Get the operator configs for the current and new operators - const currentOperatorConfig = this.configManager - .getConfigByProperty(conditionBeforeUpdate.property) - ?.getOperatorConfig(conditionBeforeUpdate.operator); - const newOperatorConfig = this.configManager - .getConfigByProperty(conditionBeforeUpdate.property) - ?.getOperatorConfig(operator); + const currentOperatorConfig = propertyConfig.getOperatorConfig(conditionBeforeUpdate.operator); + const newOperatorConfig = propertyConfig.getOperatorConfig(operator);
421-432: Improve error handling inclearFiltersmethod.The method uses
console.warnfor error cases but doesn't provide enough context about why the operation failed. Consider throwing an error or returning a result object.clearFilters: IFilterInstance<P, E>["clearFilters"] = action(async () => { if (this.canClearFilters) { const shouldNotify = shouldNotifyChangeForExpression(this.expression); this.expression = null; - await this.clearFilterOptions?.onFilterClear(); + try { + await this.clearFilterOptions?.onFilterClear(); + } catch (error) { + console.error("Failed to clear filters:", error); + // Restore expression on failure + throw error; + } if (shouldNotify) { this._notifyExpressionChange(); } } else { - console.warn("Cannot clear filters: invalid expression or missing options."); + const reasons = []; + if (!this.expression) reasons.push("no active expression"); + if (this.allConditionsForDisplay.length === 0) reasons.push("no conditions"); + if (this.clearFilterOptions?.isDisabled) reasons.push("option disabled"); + console.warn(`Cannot clear filters: ${reasons.join(", ")}`); } });
480-482: computedFn already memoizes; only optimize the deep-clone if profiling shows it’s necessaryprivate _getExternalExpression = computedFn(() => this.adapter.toExternal(sanitizeAndStabilizeExpression(toJS(this.expression))) );Location: packages/shared-state/src/store/rich-filters/filter.ts:480-482
computedFn caches the result by tracking MobX observables accessed inside the function. _getExternalExpression is used in saveView, updateView and _notifyExpressionChange; the costly part is toJS (deep clone) which only runs when the expression observables change. Suggested optional refactors (only if you observe perf issues): avoid full toJS traversal (read only needed fields), memoize sanitizeAndStabilizeExpression/adapter.toExternal, or compute/cache the external representation once when the expression changes.
packages/types/src/rich-filters/expression.ts (3)
5-13: Docs claim OR/NOT, but the model only supports AND groups.Comments mention AND/OR/NOT, while types alias groups to AND-only. Align docs or add OR/NOT node types before exposing them.
Also applies to: 75-80
66-73: Consider enforcing “≥2 children” at the type level for groups.The comment states minimum 2, but the type allows any length. Optional but helps correctness.
Add a helper and tighten the children type:
// add near other shared types type AtLeastTwo<T> = [T, T, ...T[]];- children: TFilterExpression<P>[]; + children: AtLeastTwo<TFilterExpression<P>>;
31-34: Make base node identity immutable.Prevent accidental mutation of id/type; payload updates already exclude them.
-type TBaseFilterNode = { - id: string; - type: TFilterNodeType; -}; +type TBaseFilterNode = { + readonly id: string; + readonly type: TFilterNodeType; +};packages/utils/src/rich-filters/operations/comparison.ts (2)
44-56: Fix childrenCount logic; avoid referencing non-existent child property.
child.childis never set in comparables; usechildren?.lengthonly.- if (child?.type === FILTER_NODE_TYPE.GROUP) { - const childrenCount = child.child ? 1 : Array.isArray(child.children) ? child.children.length : 0; - return `group_${child.logicalOperator}_${childrenCount}_${JSON.stringify(child)}`; - } + if (child?.type === FILTER_NODE_TYPE.GROUP) { + const childrenCount = Array.isArray(child.children) ? child.children.length : 0; + return `group_${child.logicalOperator}_${childrenCount}_${JSON.stringify(child)}`; + }Also consider replacing the
JSON.stringify(child)portion with a stable key generator to reduce sort cost on large trees.
35-37: Comment nit: only AND groups are supported today.Avoid mentioning OR in this helper to prevent confusion.
- * Helper function to create comparable children for AND/OR groups. + * Helper function to create comparable children for AND groups.packages/utils/src/rich-filters/factories/configs/core.ts (1)
102-114: Remove redundant operatorLabel reassignment.
...configalready carriesoperatorLabel; re‑setting it is a no‑op.createFilterFieldConfig<typeof FILTER_FIELD_TYPE.MULTI_SELECT, TValue>({ type: FILTER_FIELD_TYPE.MULTI_SELECT, ...DEFAULT_MULTI_SELECT_FILTER_TYPE_CONFIG, ...config, - operatorLabel: config?.operatorLabel, getOptions: () => transforms.items.map((item) => ({packages/shared-state/src/store/rich-filters/filter-helpers.ts (4)
118-133: Restructure helper ignores isNegation and does not restructure groups.Doc says it handles positive/negative operator changes; current impl only updates
operator(and optionallyvalue). Clarify doc or implement the restructuring (e.g., wrap in NOT group if/when supported).
75-78: Avoid double cloning (cloneDeep + toJS).
toJSalready deep‑clones MobX observables and plain objects. DropcloneDeepto cut overhead.- return this.adapter.toInternal(toJS(cloneDeep(initialExpression))); + return this.adapter.toInternal(toJS(initialExpression));
164-171: Tighten unsupported-operator path.Warn‑and‑return can hide bugs. Either narrow accepted operators at the type boundary or make the default branch unmistakably unreachable.
- default: - console.warn(`Unsupported logical operator: ${groupOperator}`); - return expression; + default: { + // Future: support OR/NOT. For now, return unchanged but surface loudly in dev. + if (process.env.NODE_ENV !== "production") { + // eslint-disable-next-line no-console + console.warn(`Unsupported logical operator: ${String(groupOperator)}`); + } + return expression; + }
51-54: JSDoc generic tag mismatch (K vs P).Use
Pto match the class generic.- * @template K - The filter property type extending TFilterProperty + * @template P - The filter property type extending TFilterPropertypackages/utils/src/rich-filters/operations/traversal/core.ts (2)
146-158: Remove impossible null check.
expressionis non‑nullable in this signature.export const findImmediateParent = <P extends TFilterProperty>( expression: TFilterExpression<P>, targetId: string ): TFilterGroupNode<P> | null => { - // if the expression is null, return null - if (!expression) return null; - // find the parent chain const parentChain = findParentChain(expression, targetId);
91-103: Optional: switch to early‑exit search to avoid full-tree allocation.
findNodeByIdcurrently walks the whole tree and allocates an array. A DFS with early return is simpler and faster.export const findNodeById = <P extends TFilterProperty>( expression: TFilterExpression<P>, targetId: string ): TFilterExpression<P> | null => { - const results = traverseExpressionTree( - expression, - (node) => (node.id === targetId ? node : null), - TreeTraversalMode.ALL - ); - - // Return the first match (there should only be one with unique IDs) - return results.length > 0 ? results[0] : null; + if (expression.id === targetId) return expression; + if (isGroupNode(expression)) { + const children = getGroupChildren(expression); + for (const child of children) { + const found = findNodeById(child, targetId); + if (found) return found; + } + } + return null; };packages/types/src/rich-filters/derived/core.ts (2)
16-22: Consider simplifying the complex mapped type for better readability.The nested mapped type with immediate indexing pattern could be more readable. Consider extracting the mapped type into a separate type alias for clarity.
You could improve readability with:
+type MappedOperatorHelpers<V extends TFilterValue, TConfig> = { + [K in keyof TCoreOperatorSpecificConfigs<V>]: TFilterOperatorHelper< + TCoreOperatorSpecificConfigs<V>, + K, + TConfig + >; +}; + export type TCoreSupportedSingleDateFilterOperators<V extends TFilterValue = TFilterValue> = - { - [K in keyof TCoreOperatorSpecificConfigs<V>]: TFilterOperatorHelper< - TCoreOperatorSpecificConfigs<V>, - K, - TDateFilterFieldConfig<V> - >; - }[keyof TCoreOperatorSpecificConfigs<V>]; + MappedOperatorHelpers<V, TDateFilterFieldConfig<V>>[keyof TCoreOperatorSpecificConfigs<V>];
42-43: Consider removing redundant type aliases.The
ForDisplaytype aliases (TCoreAllAvailableDateFilterOperatorsForDisplayandTCoreAllAvailableSelectFilterOperatorsForDisplay) are just re-exports without any transformation. Unless there's a specific semantic reason or future extension plan, these could be removed to reduce the API surface.Also applies to: 76-77
packages/shared-state/src/store/rich-filters/config-manager.ts (3)
108-110: Unnecessary type cast in getConfigByProperty.The cast to
IFilterConfig<P, TFilterValue>appears redundant since the Map is already typed correctly.Remove the unnecessary cast:
getConfigByProperty: IFilterConfigManager<P>["getConfigByProperty"] = computedFn( - (property) => this.filterConfigs.get(property) as IFilterConfig<P, TFilterValue> + (property) => this.filterConfigs.get(property) );
170-172: Improve null safety in config options initialization.The conditional check for
DEFAULT_FILTER_CONFIG_OPTIONSseems unusual. If it's falsy, you return potentially undefinedoptions, which might not matchTConfigOptionstype.Consider this safer approach:
private _initializeConfigOptions(options?: Partial<TConfigOptions>): TConfigOptions { - return DEFAULT_FILTER_CONFIG_OPTIONS ? { ...DEFAULT_FILTER_CONFIG_OPTIONS, ...options } : options; + return { ...DEFAULT_FILTER_CONFIG_OPTIONS, ...options } ?? {}; }
120-129: Consider adding validation for config updates.The
registermethod mutates existing configs without validation. Consider adding checks to ensure critical properties likeidaren't changed during updates.Add validation to prevent id changes:
register: IFilterConfigManager<P>["register"] = action((configUpdates) => { if (this.filterConfigs.has(configUpdates.id)) { // Update existing config if it has differences const existingConfig = this.filterConfigs.get(configUpdates.id)!; + // Ensure id cannot be changed + const { id, ...safeUpdates } = configUpdates; + if (id !== existingConfig.id) { + console.warn(`Cannot change config id from ${existingConfig.id} to ${id}`); + } - existingConfig.mutate(configUpdates); + existingConfig.mutate(safeUpdates); } else { // Create new config if it doesn't exist this.filterConfigs.set(configUpdates.id, new FilterConfig(configUpdates)); } });packages/utils/src/rich-filters/operations/transformation/core.ts (3)
32-42: Remove duplicate JSDoc comment.There's a duplicate JSDoc block for the
createGroupTransformResultfunction (lines 24-31 and 32-35).Remove the duplicate comment block:
-/** - * Generic recursive tree transformer that handles common tree manipulation logic. - * This function provides a reusable way to transform expression trees while maintaining - * tree integrity, handling group restructuring, and applying stabilization. - * - * @param expression - The expression to transform - * @param transformFn - Function that defines the transformation logic for each node - * @returns The transformation result with expression and metadata - */ /** * Helper function to create a consistent transformation result for group nodes. * Centralizes the logic for wrapping group expressions and tracking notifications. */
75-78: Type assertion could be avoided with better typing.The type assertion
as TFilterGroupNode<P>on line 78 seems necessary due to the spread operator, but could potentially be avoided with more specific typing.Consider using a type-safe helper function instead:
- const updatedGroup: TFilterGroupNode<P> = { - ...group, - children: transformedChildren, - } as TFilterGroupNode<P>; + // Add a helper function to create type-safe group updates + const updateGroupChildren = <P extends TFilterProperty>( + group: TFilterGroupNode<P>, + children: TFilterExpression<P>[] + ): TFilterGroupNode<P> => ({ + ...group, + children + } as TFilterGroupNode<P>); + + const updatedGroup = updateGroupChildren(group, transformedChildren);
119-119: Consider more specific error message.The error message could be more helpful by including the actual node type encountered.
Improve the error message:
- throw new Error("Unknown expression type in transformExpressionTree"); + throw new Error(`Unknown expression type in transformExpressionTree: ${(expression as any).type}`);packages/types/src/index.ts (1)
27-27: TODO comment needs attention.The TODO comment on line 27 indicates temporary exports from "./issues/base" that should be removed after stabilization.
Would you like me to help track this technical debt by creating an issue to ensure this TODO is addressed after the refactor/mobx-store-issue branch is stable?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/utils/src/rich-filters/operations/manipulation/core.ts (1)
92-97: Resolved: safeguard prevents corrupting group nodes.The guard against applying condition updates to groups addresses the prior concern.
packages/utils/src/rich-filters/values/core.ts (1)
7-15: Fix unsafe cast; filter null/undefined while preserving falsy values (0, false, "")Return type promises NonNullable[] but current casts can leave null/undefined when the input is an array — explicitly filter null/undefined.
File: packages/utils/src/rich-filters/values/core.ts (lines ~7-15)
-export const toFilterArray = <V extends TFilterValue>(value: SingleOrArray<V>): NonNullable<V>[] => { - if (value === null || value === undefined) { - return []; - } - - return Array.isArray(value) - ? (value as NonNullable<V>[]) - : ([value] as NonNullable<V>[]); -}; +export const toFilterArray = <V extends TFilterValue>(value: SingleOrArray<V>): NonNullable<V>[] => { + if (value == null) return []; + const arr = Array.isArray(value) ? value : [value]; + return arr.filter((v): v is NonNullable<V> => v != null); +};Add unit tests for: 0, false, "", null, undefined, [0, 1], [false, true], ["", "x"].
🧹 Nitpick comments (13)
packages/utils/src/rich-filters/operations/manipulation/core.ts (7)
2-8: Use type-only imports to avoid runtime side effects.These are types only; switch to
import typeto keep bundles lean and prevent accidental value imports.Apply:
-import { +import type { TFilterConditionPayload, TFilterExpression, TFilterGroupNode, TFilterProperty, TFilterValue, } from "@plane/types";
35-38: Avoid in-place mutation of group children (preserve referential integrity).
pushmutates the existing node; prefer returning a new object/array for consistent immutability with other helpers.Apply:
- expression.children.push(condition); - return expression; + return { + ...expression, + children: [...expression.children, condition], + };
43-46: Replace unreachable/weak error path with a thrown error.Given current unions, this branch shouldn’t be hit. If it is, fail fast.
Apply:
- console.error("Invalid expression type", expression); - return expression; + throw new Error("addAndCondition: invalid expression type");
22-25: Parameter naming/type mismatch with behavior.You accept
condition: TFilterExpression, which can be a group. Either rename tonodeor narrow to a condition node.
100-103: Tiny tidy-up: inline the children loop.Removes the temporary variable without changing behavior.
Apply:
- const children = getGroupChildren(node); - children.forEach((child) => updateNode(child)); + getGroupChildren(node).forEach(updateNode);
77-87: Document mutation semantics (in-place).This function mutates the passed expression tree. Callers need to know for state/update semantics.
Add to JSDoc: “Mutates the expression tree in place; does not return a new reference.”
115-124:preserveNotGroupsis ignored downstream; unwrapping may be too aggressive.
shouldUnwrapGroupcurrently ignores itspreserveNotGroupsflag (see validators/shared.ts snippet), so NOT groups would unwrap when they shouldn’t.Proposed fix in
packages/utils/src/rich-filters/validators/shared.ts:-export const shouldUnwrapGroup = <P extends TFilterProperty>(group: TFilterGroupNode<P>, _preserveNotGroups = true) => { +export const shouldUnwrapGroup = <P extends TFilterProperty>(group: TFilterGroupNode<P>, preserveNotGroups = true) => { const children = getGroupChildren(group); if (children.length !== 1) return false; - // Unwrap AND/OR groups with single children, and NOT groups if preserveNotGroups is false - return true; + // If this is a NOT group and preservation is requested, do not unwrap + // (Pseudo: adjust predicate to your group shape/helpers) + // if (isNotGroupNode(group) && preserveNotGroups) return false; + return true; }If NOT groups aren’t modeled yet (types re-export AND only), either wire the flag through now or remove the param to avoid misleading APIs.
packages/utils/src/rich-filters/values/core.ts (1)
20-26: Make length calculation consistent with filtered values.Currently counts null/undefined; can mis-drive UI (e.g., defaultOpen). Use toFilterArray length.
-export const getFilterValueLength = <V extends TFilterValue>(value: SingleOrArray<V>): number => { - if (value === null || value === undefined) { - return 0; - } - - return Array.isArray(value) ? value.length : 1; -}; +export const getFilterValueLength = <V extends TFilterValue>(value: SingleOrArray<V>): number => { + return toFilterArray(value).length; +};packages/shared-state/src/store/rich-filters/config-manager.ts (4)
95-99: Consider a more descriptive variable name for_allEnabledConfigs.The underscore prefix
_allEnabledConfigssuggests it's private, but it's actually referenced in a public computed property. Consider renaming to better reflect its filtered nature.- return this._allEnabledConfigs.filter((config) => config.allowMultipleFilters || !appliedProperties.has(config.id)); + return this.enabledConfigs.filter((config) => config.allowMultipleFilters || !appliedProperties.has(config.id));Also update the corresponding private getter:
- private get _allEnabledConfigs(): IFilterConfig<P, TFilterValue>[] { + private get enabledConfigs(): IFilterConfig<P, TFilterValue>[] {
108-110: Consider adding null-safety check for type assertion.The type assertion
as IFilterConfig<P, TFilterValue>could be unsafe if the map doesn't contain the expected type. While the current implementation ensures type consistency, it's better to be explicit.getConfigByProperty: IFilterConfigManager<P>["getConfigByProperty"] = computedFn( - (property) => this.filterConfigs.get(property) as IFilterConfig<P, TFilterValue> + (property) => this.filterConfigs.get(property) );The return type is already defined in the interface, so the assertion is redundant.
144-147: Add null check before mutating config.The optional chaining
prevConfig?.mutatesilently fails if the config doesn't exist. Consider explicitly handling this case to help with debugging.updateConfigByProperty: IFilterConfigManager<P>["updateConfigByProperty"] = action((property, configUpdates) => { const prevConfig = this.filterConfigs.get(property); - prevConfig?.mutate(configUpdates); + if (!prevConfig) { + console.warn(`Config for property "${property}" not found. Register it first.`); + return; + } + prevConfig.mutate(configUpdates); });
170-172: Potential edge case with empty DEFAULT_FILTER_CONFIG_OPTIONS.The ternary operator logic seems inverted - if
DEFAULT_FILTER_CONFIG_OPTIONSis truthy (even if empty object), it spreads both defaults and options. But if it's falsy, it returnsoptions || {}. This could lead to unexpected behavior.private _initializeConfigOptions(options?: Partial<TConfigOptions>): TConfigOptions { - return DEFAULT_FILTER_CONFIG_OPTIONS ? { ...DEFAULT_FILTER_CONFIG_OPTIONS, ...options } : options || {}; + return { ...DEFAULT_FILTER_CONFIG_OPTIONS, ...(options || {}) }; }packages/types/src/index.ts (1)
26-26: Remove TODO comment after completing the refactor.The TODO comment indicates temporary technical debt. Track this for removal once the refactor/mobx-store-issue branch is stable.
Would you like me to create an issue to track the removal of this temporary export once the refactor is complete?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/web/core/components/dropdowns/date.tsx(4 hunks)packages/constants/src/rich-filters/option.ts(1 hunks)packages/shared-state/package.json(1 hunks)packages/shared-state/src/store/rich-filters/config-manager.ts(1 hunks)packages/types/src/index.ts(1 hunks)packages/utils/src/datetime.ts(3 hunks)packages/utils/src/rich-filters/operations/manipulation/core.ts(1 hunks)packages/utils/src/rich-filters/values/core.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/core/components/dropdowns/date.tsx
- packages/shared-state/package.json
- packages/constants/src/rich-filters/option.ts
- packages/utils/src/datetime.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/utils/src/rich-filters/values/core.ts (2)
packages/types/src/rich-filters/expression.ts (1)
TFilterValue(24-24)packages/types/src/utils.ts (1)
SingleOrArray(9-9)
packages/shared-state/src/store/rich-filters/config-manager.ts (6)
packages/types/src/rich-filters/expression.ts (2)
TFilterProperty(19-19)TFilterValue(24-24)packages/shared-state/src/store/rich-filters/config.ts (2)
IFilterConfig(28-46)FilterConfig(48-212)packages/constants/src/rich-filters/option.ts (2)
TConfigOptions(6-6)DEFAULT_FILTER_CONFIG_OPTIONS(11-11)packages/types/src/rich-filters/config/filter-config.ts (1)
TFilterConfig(11-18)packages/types/src/rich-filters/adapter.ts (1)
TExternalFilter(7-7)packages/shared-state/src/store/rich-filters/filter.ts (1)
IFilterInstance(63-109)
packages/utils/src/rich-filters/operations/manipulation/core.ts (6)
packages/types/src/rich-filters/expression.ts (5)
TFilterProperty(19-19)TFilterExpression(86-88)TFilterConditionPayload(95-98)TFilterValue(24-24)TFilterGroupNode(79-79)packages/utils/src/rich-filters/types/core.ts (3)
isConditionNode(19-21)isGroupNode(28-29)isAndGroupNode(36-38)packages/utils/src/rich-filters/factories/nodes/core.ts (1)
createAndGroupNode(32-39)packages/utils/src/rich-filters/operations/transformation/core.ts (1)
transformExpressionTree(92-120)packages/utils/src/rich-filters/types/shared.ts (1)
getGroupChildren(37-40)packages/utils/src/rich-filters/validators/shared.ts (1)
shouldUnwrapGroup(12-22)
🪛 GitHub Actions: Build and lint web apps
packages/utils/src/rich-filters/values/core.ts
[warning] 1-1: Prettier formatting issue detected in this file. Run 'prettier --write' to fix.
🔇 Additional comments (4)
packages/utils/src/rich-filters/operations/manipulation/core.ts (1)
61-75: Confirm droppingshouldNotifyis intentional.
transformExpressionTreeproducesshouldNotify, but it’s discarded here. If callers rely on notifications, ensure they’re handled upstream.packages/utils/src/rich-filters/values/core.ts (1)
1-27: Fix Prettier to unblock CI.The job reports a formatting issue. Run Prettier on this file (or the repo).
Example:
- pnpm:
pnpm prettier --write packages/utils/src/rich-filters/values/core.ts- or:
prettier --write packages/utils/src/rich-filters/values/core.tspackages/shared-state/src/store/rich-filters/config-manager.ts (1)
120-129: LGTM! Proper use of MobX action for state mutations.The implementation correctly uses MobX action decorator and properly handles both update and creation scenarios.
packages/types/src/index.ts (1)
1-50: Export structure looks good - comprehensive coverage of new rich-filters module.The reorganization properly exports all necessary modules including the new rich-filters feature. The alphabetical ordering aids in maintainability.
| } | ||
|
|
||
| return Array.isArray(value) ? (value as NonNullable<V>[]) : ([value] as NonNullable<V>[]); | ||
| }; |
There was a problem hiding this comment.
Bug: Function Fails to Filter Null Values
The toFilterArray function incorrectly asserts its return type as NonNullable<V>[]. When the input value is an array, it doesn't filter out null or undefined elements, allowing them to remain in the returned array. This violates the type contract and can lead to runtime errors for consumers expecting non-nullable values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/utils/src/rich-filters/types/shared.ts (4)
27-31: Docstring claims OR/NOT support that isn’t implemented.The comment mentions AND/OR and NOT, but code (and types) only handle AND groups. Please correct to avoid misleading consumers.
- * Gets the children of a group node, handling AND/OR groups (children array) and NOT groups (single child). - * Uses processGroupNode for consistent group type handling. + * Gets the children of a group node. + * Currently supports AND groups only. Extend when OR/NOT groups are introduced. + * Uses processGroupNode for consistent group type handling.
16-24: Improve error context; current template prints “[object Object]”.Interpolate the logical operator (and optionally a brief shape) instead of the whole object so errors are actionable.
export const processGroupNode = <P extends TFilterProperty, T>( group: TFilterGroupNode<P>, handlers: TProcessGroupNodeHandlers<P, T> ): T => { if (isAndGroupNode(group)) { return handlers.onAndGroup(group); } - throw new Error(`Invalid group node: unknown logical operator ${group}`); + const op = (group as any)?.logicalOperator ?? 'unknown'; + throw new Error(`Invalid group node: unknown logical operator: ${String(op)}`); };Follow‑up: when OR/NOT are added, consider an exhaustive check pattern (assertNever) to make unhandled cases a compile‑time error.
6-8: Clarify handler surface and future‑proof the type.Add a short doc that today only AND is required; more handlers will be added as new group kinds land.
+/** + * Handlers for group node processing. + * Note: currently only AND groups exist; extend this type when OR/NOT are introduced. + */ type TProcessGroupNodeHandlers<P extends TFilterProperty, T> = { onAndGroup: (group: TFilterAndGroupNode<P>) => T; };
32-35: Tiny tidy + optional immutability.
- Pass function ref directly.
- Optionally return a defensive copy or a readonly type to prevent accidental mutation by callers.
-export const getGroupChildren = <P extends TFilterProperty>(group: TFilterGroupNode<P>): TFilterExpression<P>[] => - processGroupNode(group, { - onAndGroup: (andGroup) => getAndGroupChildren(andGroup), - }); +export const getGroupChildren = <P extends TFilterProperty>( + group: TFilterGroupNode<P> +): TFilterExpression<P>[] => + processGroupNode(group, { + onAndGroup: getAndGroupChildren, + });If you prefer immutability:
-export const getGroupChildren = <P extends TFilterProperty>(group: TFilterGroupNode<P>): TFilterExpression<P>[] => +export const getGroupChildren = <P extends TFilterProperty>(group: TFilterGroupNode<P>): ReadonlyArray<TFilterExpression<P>> => processGroupNode(group, { - onAndGroup: getAndGroupChildren, + onAndGroup: (andGroup) => [...getAndGroupChildren(andGroup)], });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/utils/src/rich-filters/types/shared.ts(1 hunks)packages/utils/src/rich-filters/values/core.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utils/src/rich-filters/values/core.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/utils/src/rich-filters/types/shared.ts (2)
packages/types/src/rich-filters/expression.ts (4)
TFilterProperty(19-19)TFilterAndGroupNode(69-73)TFilterGroupNode(79-79)TFilterExpression(86-88)packages/utils/src/rich-filters/types/core.ts (2)
isAndGroupNode(36-38)getAndGroupChildren(57-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
…e#7802) * feat: add rich filters types * feat: add rich filters constants * feat: add rich filters utils * feat: add rich filters store in shared state package * feat: add rich filters UI components * fix: make setLoading optional in loadOptions function for improved flexibility * chore: minor improvements to rich filters * fix: formatting
2 participants
Description
Type of Change
Summary by CodeRabbit