✨AI Column: Create cache mechanism#31569
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements caching functionality for AI column data in the DevExtreme grid. The changes transform stub methods in the AIColumnCacheController into a working in-memory cache implementation using nested Map structures.
Key changes:
- Implemented cache storage using
Map<string, Map<PropertyKey, string>>to store AI column responses - Added cache persistence after successful AI column requests
- Connected cache operations to the controller methods for retrieval and clearing
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| m_ai_column_cache_controller.ts | Implemented in-memory cache with Map-based storage and full CRUD operations |
| m_ai_column_integration_controller.ts | Added cache persistence after receiving AI responses and exposed cache retrieval/clearing methods |
| m_ai_column_controller.ts | Updated parameter type from any to unknown and delegated cache operations to integration controller |
Comments suppressed due to low confidence (2)
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_integration_controller.ts:200
- The
disposemethod doesn't dispose of theaiColumnCacheControllerinstance. SinceaiColumnCacheControllerhas its owndisposemethod that clears the cache, it should be called here to prevent memory leaks:this.aiColumnCacheController.dispose();
public dispose(): void {
super.dispose();
Object.keys(this.aborts).forEach((columnName) => this.abortRequest(columnName));
}
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_controller.ts:110
- The
disposemethod doesn't dispose of theaiColumnIntegrationControllerinstance. This should callthis.aiColumnIntegrationController.dispose();to properly clean up resources and prevent memory leaks.
public dispose(): void {
this.dataController.changed.remove(this.dataChangedHandler);
}
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_cache_controller.ts
Outdated
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_cache_controller.ts
Outdated
Show resolved
Hide resolved
...ges/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_integration_controller.ts
Outdated
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_controller.ts
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_cache_controller.ts
Outdated
Show resolved
Hide resolved
...ges/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_integration_controller.ts
Outdated
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_cache_controller.ts
Outdated
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_controller.ts
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_cache_controller.ts
Show resolved
Hide resolved
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_cache_controller.ts
Show resolved
Hide resolved
| let cachedResponse: Record<PropertyKey, string> = {}; | ||
| if (args.useCache) { | ||
| const keys = data.map((item) => item[keyField] as PropertyKey); | ||
| cachedResponse = this.aiColumnCacheController.getCachedResponse(columnName, keys); | ||
| } |
There was a problem hiding this comment.
[nitpick] The variable declaration and initialization could be simplified. Consider declaring cachedResponse inside the if block or using a more explicit initialization pattern. The current approach initializes an empty object that gets immediately reassigned when args.useCache is true.
| let cachedResponse: Record<PropertyKey, string> = {}; | |
| if (args.useCache) { | |
| const keys = data.map((item) => item[keyField] as PropertyKey); | |
| cachedResponse = this.aiColumnCacheController.getCachedResponse(columnName, keys); | |
| } | |
| const cachedResponse: Record<PropertyKey, string> = args.useCache | |
| ? this.aiColumnCacheController.getCachedResponse( | |
| columnName, | |
| data.map((item) => item[keyField] as PropertyKey), | |
| ) | |
| : {}; |
packages/devextreme/js/__internal/grids/grid_core/ai_column/m_ai_column_cache_controller.ts
Show resolved
Hide resolved
| this.cache[columnName] = columnCache; | ||
| } | ||
| Object.entries(data).forEach(([key, value]) => { | ||
| if (columnCache && value !== '') { |
There was a problem hiding this comment.
Inconsistent handling of empty string values. In getCachedResponse, empty strings are filtered out during retrieval (line 16), but in setCachedResponse, they're prevented from being stored. This dual filtering is redundant and could mask issues. Consider standardizing to handle empty strings in one location only.
| super.dispose(); | ||
| if (this.aiColumnOptionChangedHandler) { | ||
| this.columnsController.aiColumnOptionChanged.remove(this.aiColumnOptionChangedHandler); | ||
| } | ||
| if (this.dataChangedHandler) { | ||
| this.dataController.changed.remove(this.dataChangedHandler); | ||
| } |
There was a problem hiding this comment.
The super.dispose() call should typically come after cleaning up the component's own resources, not before. This ensures that if the parent's dispose method has any dependencies on the current state, they're still available. Consider moving super.dispose() to the end of the method.
| super.dispose(); | |
| if (this.aiColumnOptionChangedHandler) { | |
| this.columnsController.aiColumnOptionChanged.remove(this.aiColumnOptionChangedHandler); | |
| } | |
| if (this.dataChangedHandler) { | |
| this.dataController.changed.remove(this.dataChangedHandler); | |
| } | |
| if (this.aiColumnOptionChangedHandler) { | |
| this.columnsController.aiColumnOptionChanged.remove(this.aiColumnOptionChangedHandler); | |
| } | |
| if (this.dataChangedHandler) { | |
| this.dataController.changed.remove(this.dataChangedHandler); | |
| } | |
| super.dispose(); |
| super.dispose(); | ||
|
|
||
| if (this.aiColumnOptionChangedHandler) { | ||
| this.columnsController.aiColumnOptionChanged.remove(this.aiColumnOptionChangedHandler); | ||
| } |
There was a problem hiding this comment.
Similar to AIColumnController, the super.dispose() call should typically come after cleaning up component-specific resources. Consider moving super.dispose() to the end of the dispose method.
| super.dispose(); | |
| if (this.aiColumnOptionChangedHandler) { | |
| this.columnsController.aiColumnOptionChanged.remove(this.aiColumnOptionChangedHandler); | |
| } | |
| if (this.aiColumnOptionChangedHandler) { | |
| this.columnsController.aiColumnOptionChanged.remove(this.aiColumnOptionChangedHandler); | |
| } | |
| super.dispose(); |
| super.dispose(); | ||
|
|
||
| if (this.aiColumnOptionChangedHandler) { | ||
| this.columnsController.aiColumnOptionChanged.remove(this.aiColumnOptionChangedHandler); |
There was a problem hiding this comment.
It's not necessary to remove this method from the callback list. This is done at the base level.
No description provided.