feat: Allow editing saved filters in data browser #2942
Conversation
This reverts commit b66beb8.
|
🚀 Thanks for opening this pull request! |
|
Caution Review failedThe pull request is closed. 📝 Walkthrough""" WalkthroughThis set of changes introduces comprehensive enhancements to filter management within the data browser UI. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BrowserFilter
participant Browser
participant Preferences
participant URL
User->>BrowserFilter: Edit or copy filter (name, relative date, etc.)
BrowserFilter->>Browser: saveFilters(filters, name, relativeDate, [filterId])
Browser->>Preferences: Update saved filters (add/edit)
Browser->>BrowserFilter: Return filterId
BrowserFilter->>URL: Update with filterId (if new/edited)
User->>BrowserFilter: Delete filter
BrowserFilter->>Browser: deleteFilter(filterId)
Browser->>Preferences: Remove filter by filterId
Browser->>BrowserFilter: Confirm deletion
BrowserFilter->>URL: Remove filterId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERR_SSL_WRONG_VERSION_NUMBER 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/dashboard/Data/Browser/Browser.react.js (1)
29-29: Consider renaming the importedMapto avoid shadowing.The import of
Mapfrom Immutable shadows the global JavaScriptMapconstructor. Consider using a named import alias to avoid confusion:-import { List, Map } from 'immutable'; +import { List, Map as ImmutableMap } from 'immutable';Then update all usages of
Mapin the file toImmutableMap.src/components/BrowserFilter/BrowserFilter.react.js (1)
231-253: Simplify filter comparison using Immutable.js methodsSince you're using Immutable.js, you can simplify the comparison logic by leveraging its built-in equality checking.
Replace the manual property comparison with:
- // Compare each filter - for (let i = 0; i < currentFilters.size; i++) { - const currentFilter = currentFilters.get(i); - const originalFilter = originalFilters.get(i); - - // Compare each property of the filter - const currentClass = currentFilter.get('class'); - const currentField = currentFilter.get('field'); - const currentConstraint = currentFilter.get('constraint'); - const currentCompareTo = currentFilter.get('compareTo'); - - const originalClass = originalFilter.get('class'); - const originalField = originalFilter.get('field'); - const originalConstraint = originalFilter.get('constraint'); - const originalCompareTo = originalFilter.get('compareTo'); - - // Check all properties for equality - if (currentClass !== originalClass || - currentField !== originalField || - currentConstraint !== originalConstraint || - currentCompareTo !== originalCompareTo) { - return true; - } - } - - return false; + // Use Immutable.js equality check + return !currentFilters.equals(originalFilters);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/BrowserFilter/BrowserFilter.react.js(6 hunks)src/components/Button/Button.react.js(2 hunks)src/components/Button/Button.scss(1 hunks)src/components/CategoryList/CategoryList.react.js(3 hunks)src/dashboard/Data/Browser/Browser.react.js(5 hunks)src/dashboard/Data/Browser/BrowserToolbar.react.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/dashboard/Data/Browser/Browser.react.js (2)
Learnt from: mtrezza
PR: #2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the confirmExecuteScriptRows method in src/dashboard/Data/Browser/Browser.react.js), individual setState calls to update processedScripts counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
Learnt from: mtrezza
PR: #2769
File: src/lib/ClassPreferences.js:26-26
Timestamp: 2025-05-02T11:55:52.809Z
Learning: Preference reads and writes in the ClassPreferences.js module are expected to be infrequent operations, so optimizing for performance (like caching) is unnecessary in this context.
🪛 Biome (2.1.2)
src/dashboard/Data/Browser/Browser.react.js
[error] 29-29: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/components/BrowserFilter/BrowserFilter.react.js
[error] 19-19: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (5)
src/components/Button/Button.scss (1)
80-80: LGTM!The formatting fix improves code consistency by adding the missing space before the opening brace.
src/dashboard/Data/Browser/BrowserToolbar.react.js (1)
35-35: LGTM!The addition of the
onDeleteFilterprop properly integrates filter deletion functionality into the toolbar component, enabling it to propagate delete actions from the BrowserFilter component up to the parent.Also applies to: 405-405
src/components/Button/Button.react.js (1)
10-11: Good UX improvement!The addition of the
onMouseLeavehandler effectively prevents sticky focus states on buttons, improving the user experience. The import reordering is also fine.Also applies to: 56-59
src/components/CategoryList/CategoryList.react.js (1)
9-9: Well-implemented filter ID matching with backward compatibility!The changes properly prioritize filter matching by ID while maintaining backward compatibility through the legacy fallback. This aligns well with the new filter management system. The removal of the inline close button is appropriate as filter deletion is now handled through dedicated UI flows.
Also applies to: 60-70, 124-131
src/dashboard/Data/Browser/Browser.react.js (1)
1203-1268: Well-structured filter save/update implementation!The enhanced
saveFiltersmethod properly handles both creating new filters with generated UUIDs and updating existing filters by ID. The return value allows callers to track newly created filters, and the fallback logic ensures robustness.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/dashboard/Data/Browser/Browser.react.js (1)
1270-1303: Consider renaming the misleading parameter name.The
deleteFiltermethod's parameter is namedfilterIdOrObjectbut can also accept a filter object as indicated by the fallback logic. This is confusing. Consider renaming it to better reflect its dual purpose:-deleteFilter(filterIdOrObject) { +deleteFilter(filterOrId) {Also update the references within the method accordingly. The implementation logic itself is solid with good backward compatibility support.
🧹 Nitpick comments (1)
src/dashboard/Data/Browser/Browser.react.js (1)
29-29: Address the global Map shadowing issue.The static analysis tool correctly identifies that importing
Mapfrom Immutable.js shadows the globalMapconstructor. Consider using a more explicit import to avoid confusion:-import { List, Map } from 'immutable'; +import { List, Map as ImmutableMap } from 'immutable';Then update the usage throughout the file accordingly. This makes it clear when you're using Immutable's Map versus the native Map constructor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/BrowserFilter/BrowserFilter.react.js(7 hunks)src/dashboard/Data/Browser/Browser.react.js(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/dashboard/Data/Browser/Browser.react.js (2)
Learnt from: mtrezza
PR: #2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the confirmExecuteScriptRows method in src/dashboard/Data/Browser/Browser.react.js), individual setState calls to update processedScripts counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
Learnt from: mtrezza
PR: #2769
File: src/lib/ClassPreferences.js:26-26
Timestamp: 2025-05-02T11:55:52.809Z
Learning: Preference reads and writes in the ClassPreferences.js module are expected to be infrequent operations, so optimizing for performance (like caching) is unnecessary in this context.
src/components/BrowserFilter/BrowserFilter.react.js (1)
Learnt from: mtrezza
PR: #2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the confirmExecuteScriptRows method in src/dashboard/Data/Browser/Browser.react.js), individual setState calls to update processedScripts counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
🪛 Biome (2.1.2)
src/dashboard/Data/Browser/Browser.react.js
[error] 29-29: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (18)
src/dashboard/Data/Browser/Browser.react.js (3)
230-230: LGTM!Proper method binding in the constructor for the new
deleteFiltermethod.
1203-1268: Excellent enhancement to support filter editing.The updated
saveFiltersmethod effectively handles both creating new filters and updating existing ones. Key improvements:
- Clean separation between create/update logic based on
filterIdparameter- Proper fallback when existing filter not found by ID
- Returns the filter ID for caller convenience
- Uses modern
crypto.randomUUID()for ID generation- Maintains backward compatibility
The implementation is robust and well-structured.
2298-2298: LGTM!Proper integration of the
onDeleteFilterprop to enable filter deletion functionality in the DataBrowser component.src/components/BrowserFilter/BrowserFilter.react.js (15)
19-19: LGTM! Map shadowing issue has been resolved.The import has been properly aliased as
ImmutableMapto avoid shadowing the globalMapobject, addressing the previous review concern.
40-44: Well-structured state additions for enhanced filter management.The new state variables are appropriately named and initialized to support the editing, deletion, and change tracking functionality.
55-98: Robust filter detection with appropriate fallback logic.The method correctly prioritizes URL-based filter identification and includes a sensible fallback for legacy compatibility. The error handling prevents crashes from malformed saved filters.
119-123: Error handling has been improved.The catch block now properly logs parsing errors for debugging while gracefully handling the failure case, addressing the previous review concern about silent error handling.
143-176: Well-structured toggle logic for edit mode transitions.The method properly manages state transitions, stores original values for comparison, and handles filter conversion appropriately for the enhanced editing functionality.
178-190: Proper duplicate name detection with edit-mode awareness.The method correctly excludes the current filter ID when checking for duplicates, preventing false positives during filter editing.
192-214: Comprehensive date format normalization for reliable comparisons.The method properly handles all three date formats (JavaScript Date, Parse Date, RelativeDate) and converts them to a consistent ISO string format for accurate filter comparison.
216-257: Thorough change detection with comprehensive filter comparison.The method provides detailed comparison logic that properly normalizes filters and checks all relevant properties to accurately detect content changes.
259-281: Proper date format conversion for UI display.The method correctly converts various date formats to JavaScript Date objects while preserving existing Date objects, ensuring consistent UI behavior.
283-283: Method naming has been improved for clarity.The method name has been updated from the previously misleading
convertRelativeDatesForDisplaytoconvertRelativeDatesToParseFormat, which accurately reflects its purpose of converting for saving rather than display.
299-336: Delete logic has been properly extracted into a dedicated method.The delete functionality has been moved from the inline onClick handler to this dedicated method, improving testability and maintainability as suggested in the previous review.
349-354: Proper date conversion and state management in toggle method.The addition of date conversion for display and showMore state reset appropriately supports the enhanced filter editing functionality.
405-475: Comprehensive save method enhancement for editing capabilities.The method now properly handles both new filter creation and editing existing filters, with appropriate URL management, date conversion, and UI state preservation.
544-569: Well-structured UI additions for enhanced filter editing.The conditional rendering provides appropriate input fields for filter name and relative dates checkbox, with proper field structure and labeling.
624-645: Duplicate rendering logic has been eliminated.The conditional rendering has been simplified to remove the duplicate code that was rendering identical UI elements in different branches, improving maintainability as suggested in the previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/dashboard/Data/Browser/Browser.react.js (1)
29-29: Address the global property shadowing warning.The import of
Mapfrom Immutable shadows the globalMapproperty, which can cause confusion about variable origins.Consider using a named import to avoid shadowing:
-import { List, Map } from 'immutable'; +import { List, Map as ImmutableMap } from 'immutable';Then update all usages of
MaptoImmutableMapthroughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/dashboard/Data/Browser/Browser.react.js(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/dashboard/Data/Browser/Browser.react.js (2)
Learnt from: mtrezza
PR: #2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the confirmExecuteScriptRows method in src/dashboard/Data/Browser/Browser.react.js), individual setState calls to update processedScripts counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
Learnt from: mtrezza
PR: #2769
File: src/lib/ClassPreferences.js:26-26
Timestamp: 2025-05-02T11:55:52.809Z
Learning: Preference reads and writes in the ClassPreferences.js module are expected to be infrequent operations, so optimizing for performance (like caching) is unnecessary in this context.
🪛 Biome (2.1.2)
src/dashboard/Data/Browser/Browser.react.js
[error] 29-29: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (5)
src/dashboard/Data/Browser/Browser.react.js (5)
230-230: LGTM: Proper method binding.The binding for the
deleteFiltermethod is correctly added to ensure properthiscontext.
527-544: Excellent enhancement for date filter handling.The conversion of date strings to Parse Date objects is well-implemented and essential for proper Parse query functionality. The logic correctly:
- Validates date strings using
Date.parse()- Converts to Parse Date format with
__type: 'Date'and ISO string- Preserves all other filter properties
This ensures that date filters work correctly with Parse's query system.
1216-1281: Excellent enhancement supporting filter creation and updates.The enhanced
saveFiltersmethod is well-implemented with several improvements:
- Dual functionality: Supports both creating new filters and updating existing ones via optional
filterIdparameter- Relative date handling: Properly converts date filters to relative date format when needed
- Modern ID generation: Uses
crypto.randomUUID()for unique filter identifiers- Robust fallback: Creates new filter if existing one isn't found during update
- Return value: Returns filter ID for caller integration
The relative date conversion logic correctly transforms Parse Date objects to RelativeDate format with proper time calculations.
1283-1316: Solid implementation with good backward compatibility.The
deleteFiltermethod provides robust filter deletion with excellent fallback logic:
- Primary approach: Deletes by filter ID (modern, efficient)
- Fallback approach: Falls back to object comparison for backward compatibility
- Proper cleanup: Updates preferences and forces component update
The dual approach ensures compatibility with both new ID-based filters and legacy filter objects. Good to see the previous feedback about parameter naming was already addressed in commit 5128026.
2311-2311: LGTM: Proper prop connection for filter deletion.The
onDeleteFilterprop correctly connects thedeleteFiltermethod to theDataBrowsercomponent, enabling the filter deletion functionality in the UI.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/dashboard/Data/Browser/Browser.react.js (1)
1371-1392: Fix trailing spaces while maintaining good filterId preservation logic.The filterId preservation in relation URLs follows the same pattern as updateFilters and works correctly. However, static analysis detected trailing spaces that should be removed.
Apply this diff to remove trailing spaces:
- // Preserve filterId from current URL if it exists +// Preserve filterId from current URL if it exists - const currentUrlParams = new URLSearchParams(window.location.search); +const currentUrlParams = new URLSearchParams(window.location.search); - const currentFilterId = currentUrlParams.get('filterId'); +const currentFilterId = currentUrlParams.get('filterId'); - + - // Preserve filterId if it exists in current URL +// Preserve filterId if it exists in current URL - + - +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/dashboard/Data/Browser/Browser.react.js(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/dashboard/Data/Browser/Browser.react.js (2)
Learnt from: mtrezza
PR: #2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the confirmExecuteScriptRows method in src/dashboard/Data/Browser/Browser.react.js), individual setState calls to update processedScripts counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
Learnt from: mtrezza
PR: #2769
File: src/lib/ClassPreferences.js:26-26
Timestamp: 2025-05-02T11:55:52.809Z
Learning: Preference reads and writes in the ClassPreferences.js module are expected to be infrequent operations, so optimizing for performance (like caching) is unnecessary in this context.
🪛 Biome (2.1.2)
src/dashboard/Data/Browser/Browser.react.js
[error] 29-29: Do not shadow the global "Map" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 GitHub Check: Lint
src/dashboard/Data/Browser/Browser.react.js
[failure] 1225-1225:
Trailing spaces not allowed
[failure] 1222-1222:
Trailing spaces not allowed
[failure] 1217-1217:
Trailing spaces not allowed
[failure] 1208-1208:
Trailing spaces not allowed
[failure] 1204-1204:
Trailing spaces not allowed
[failure] 1387-1387:
Trailing spaces not allowed
[failure] 1384-1384:
Trailing spaces not allowed
[failure] 1379-1379:
Trailing spaces not allowed
[failure] 1374-1374:
Trailing spaces not allowed
⏰ 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). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (6)
src/dashboard/Data/Browser/Browser.react.js (6)
8-44: Import reorganization looks good.The new imports support the enhanced filter management functionality. The static analysis warning about shadowing the global "Map" property is a false positive - this is the standard way to import Immutable's Map, which is a different entity from the global Map constructor.
230-230: LGTM!Standard method binding pattern for the new deleteFilter functionality.
527-547: Excellent enhancement for date handling in filters.The date string conversion logic properly handles the serialization/deserialization of date filters in URL parameters. Converting string dates back to Parse Date objects ensures queries work correctly when filters are loaded from URLs.
1235-1300: Well-designed dual-purpose method for filter management.The enhanced saveFilters method elegantly handles both creating new filters and updating existing ones. The relative date conversion logic is particularly valuable for maintaining time-based filters that should be dynamic. The use of crypto.randomUUID() and returning the filter ID enables proper integration with the UI.
1302-1335: Robust filter deletion with good backward compatibility.The deleteFilter method implements a solid dual-approach strategy - primary deletion by ID with fallback to object comparison for backward compatibility. The implementation properly updates preferences and refreshes the component. I note that the parameter naming concern from previous reviews has been addressed.
2340-2340: LGTM!Clean prop addition that enables filter deletion functionality in the DataBrowser component.
# [7.3.0-alpha.31](7.3.0-alpha.30...7.3.0-alpha.31) (2025-07-26) ### Features * Allow editing saved filters in data browser ([#2942](#2942)) ([daaccaa](daaccaa))
|
🎉 This change has been released in version 7.3.0-alpha.31 |
Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor
Chores