fix(table): prevent duplicate filter events on delegate reuse#975
Conversation
Resolves issue where forge-table-filter event fires multiple times based on how many times filter elements have been rendered. This occurred when the same delegate instance was reused and onChange() was called multiple times, accumulating event listeners. The fix adds a flag to prevent attaching multiple onChange listeners to the same delegate instance, ensuring each filter only fires one event regardless of how many times the table is re-rendered.
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where table filter events fire multiple times when filter elements are re-rendered. The issue occurred because onChange listeners were being attached repeatedly to the same delegate instance, accumulating event handlers each time filters were toggled or tables re-rendered.
- Adds a flag to prevent duplicate onChange listener attachment
- Ensures each filter delegate only has one active event listener
- Maintains backward compatibility with existing delegate usage patterns
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| delegate.onChange(debounce((value: any) => filterListener(value, columnIndex), debounceTime)); | ||
| } else { | ||
| delegate.onChange((value: any) => filterListener(value, columnIndex)); | ||
| if (!(delegate as any).__forgeTableListenerAttached) { |
There was a problem hiding this comment.
@Ankur-Thakur-NEU I'm not a fan of adding dynamic properties like this, especially ones that are boolean flags because things can get out of sync easily. Perhaps we could use a WeakMap and store listener references safely, and add some logic to the delegates to remove previous listeners when onChange() is called?
I'm thinking something like this at the class level:
// Track filter delegates and their associated listeners to prevent duplicates
private static _filterDelegateListeners = new WeakMap<IFormFieldComponentDelegate<any>, (value: any) => void>();Then replace the logic here with the following:
// If this is a FormFieldComponentDelegate then we can listen for when the value changes, otherwise we just render the custom delegate element
if (!!filterListener && delegate instanceof FormFieldComponentDelegate && isFunction(delegate.onChange)) {
// Check if we already have a listener for this delegate to prevent duplicates
if (!TableUtils._filterDelegateListeners.has(delegate)) {
let listener: (value: any) => void;
if (!isDefined(columnConfig.filterDebounceTime) || isNumber(columnConfig.filterDebounceTime)) {
const debounceTime = isDefined(columnConfig.filterDebounceTime)
? (columnConfig.filterDebounceTime as number)
: TABLE_CONSTANTS.numbers.DEFAULT_FILTER_DEBOUNCE_TIME;
listener = debounce((value: any) => filterListener(value, columnIndex), debounceTime);
} else {
listener = (value: any) => filterListener(value, columnIndex);
}
// Store the listener in the WeakMap and attach it
TableUtils._filterDelegateListeners.set(delegate, listener);
delegate.onChange(listener);
}
}This is untested but it should safely store the delegate instances and listeners so they can be cleaned up by GC, and hopefully net the same result. Could you try this or something similar and let me know? We should also add a test for this as well.
Resolves issue where forge-table-filter event fires multiple times based on how many times filter elements have been rendered. This occurred when the same delegate instance was reused and onChange() was called multiple times, accumulating event listeners.
The fix adds a flag to prevent attaching multiple onChange listeners to the same delegate instance, ensuring each filter only fires one event regardless of how many times the table is re-rendered.
Issue - #622
PR Checklist
Please check if your PR fulfills the following requirements:
Describe the new behavior?
Prevents forge-table-filter events from firing multiple times when filter elements are re-rendered. The fix adds a flag (__forgeTableListenerAttached) to prevent duplicate onChange listeners from being attached to the same delegate instance when filters are toggled or tables are re-rendered.
Additional information
Issue: When using direct delegate instances (e.g., filterDelegate: new TextFieldComponentDelegate(...)), the onChange() method would accumulate multiple event listeners each time filters were enabled/disabled or the table was re-rendered, causing events to fire N times where N = render count.
Solution: Minimal 3-line fix that checks if a listener is already attached before adding a new one, ensuring only one active listener per delegate instance while maintaining backward compatibility with both factory functions and direct instances.
Forge.table.filter.logging.-.Trim.mp4