Nc Fix/limit record in group by if it is external source #11257
Conversation
📝 WalkthroughWalkthroughThe changes introduce new logic to handle external source record visibility restrictions and refine UI behavior in the pagination and grid components. A new optional prop is added to the pagination component to adjust its offset based on grid mode. The expanded-form component now exposes a method to stop loading externally. The GroupByTable component implements a validation function to prevent loading more records than allowed for external sources, displaying an upgrade modal if the limit is exceeded. The Table and grid index components are updated to support these changes through new prop bindings and feature flag gating. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GroupByTable
participant ExpandedForm
participant UpgradeModal
User->>GroupByTable: Request to change page or go to next row
GroupByTable->>GroupByTable: validateExternalSourceRecordVisibility(page)
alt Page limit exceeded and is external source
GroupByTable->>UpgradeModal: showUpgradeToSeeMoreRecordsModal()
GroupByTable->>ExpandedForm: stopLoading() (if applicable)
Note right of GroupByTable: Abort page load
else Page within limit
GroupByTable->>GroupByTable: Proceed with page load
end
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue (2)
238-247: Consider moving validation logic to a composable for reusabilityThe external source validation logic might be useful in other components. Consider extracting this function to a shared composable to promote reuse and maintain consistency across the application.
-const validateExternalSourceRecordVisibility = (page: number, callback?: () => void) => { - if ( - (vGroup.value.paginationData?.pageSize ?? 10) * page > 100 && - showUpgradeToSeeMoreRecordsModal({ isExternalSource: isExternalSource.value }) - ) { - return true - } - - callback?.() -}Create a new composable like
useExternalSourceValidation.ts:export function useExternalSourceValidation(paginationData, isExternalSource) { const { showUpgradeToSeeMoreRecordsModal } = useEeConfig() const validateRecordVisibility = (page: number, callback?: () => void) => { if ( (paginationData?.pageSize ?? 10) * page > 100 && showUpgradeToSeeMoreRecordsModal({ isExternalSource: isExternalSource.value }) ) { return true } callback?.() } return { validateRecordVisibility } }Then import and use it in this component.
240-241: Magic number in validation logicThe code uses a hard-coded value of 100 as the record limit threshold. Consider extracting this to a constant or configuration value to make it more maintainable and easier to adjust if needed.
+const EXTERNAL_SOURCE_RECORD_LIMIT = 100 const validateExternalSourceRecordVisibility = (page: number, callback?: () => void) => { if ( - (vGroup.value.paginationData?.pageSize ?? 10) * page > 100 && + (vGroup.value.paginationData?.pageSize ?? 10) * page > EXTERNAL_SOURCE_RECORD_LIMIT && showUpgradeToSeeMoreRecordsModal({ isExternalSource: isExternalSource.value }) ) { return true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/nc-gui/components/smartsheet/Pagination.vue(3 hunks)packages/nc-gui/components/smartsheet/expanded-form/index.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/GroupByTable.vue(4 hunks)packages/nc-gui/components/smartsheet/grid/Table.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/index.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
🔇 Additional comments (16)
packages/nc-gui/components/smartsheet/expanded-form/index.vue (2)
709-713: Proper handling of loading state through the component lifecycle.This new
stopLoadingmethod provides a way to externally control the loading state of the expanded form, ensuring DOM updates are completed before changing the visibility state.
715-717: Exposing component functionality for external control.Good practice to expose only the necessary methods through
defineExpose. This allows parent components to manage the loading state of this component as needed.packages/nc-gui/components/smartsheet/Pagination.vue (3)
17-17: Added prop for pagination positioning control.The addition of this boolean prop enhances component flexibility by allowing the parent to control pagination positioning based on grid mode.
32-32: Reactive reference extraction for the new prop.Correctly including the new prop in the reactive references to maintain reactivity throughout the component.
117-119: Refined pagination positioning for different grid modes.This change improves the UI layout by specifying different left offset classes based on the grid mode. When in group-by view with an add-record slot:
- Uses
left-[159px]when in add-new-record grid mode- Uses
left-[199px]otherwiseThis ensures proper alignment of the pagination controls with other UI elements based on the context.
packages/nc-gui/components/smartsheet/grid/Table.vue (1)
2730-2730: Forwarding grid mode prop to pagination component.Properly passes the
isAddNewRecordGridModestate to the pagination component, ensuring consistent UI behavior between the grid and pagination components.packages/nc-gui/components/smartsheet/grid/index.vue (3)
18-18: Added external source flag from smartsheet store.This change allows checking whether the current data source is external, which is necessary for implementing the record visibility restriction logic.
24-25: Added external source record visibility restriction function.Importing the
blockExternalSourceRecordVisibilityfunction from the EE config provides the mechanism to enforce visibility restrictions when needed.
210-212: Gating Canvas Group By Table feature for external sources.This change enhances security by conditionally disabling the Canvas Group By Table feature when restrictions should be applied to external data sources. It's a key part of fixing the issue mentioned in the PR title about limiting records in group by operations for external sources.
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue (7)
45-47: Added utility imports for external source validationThe addition of
isExternalSourceandshowUpgradeToSeeMoreRecordsModalprovides the necessary tools to implement record visibility restrictions for external data sources. This is a clean approach to feature gating based on source type.
53-53: New reference to expose expanded form methodsAdding
expandedFormRefenables access to the expanded form component's methods, particularlystopLoading, which is used when external source validation fails.
238-247: Well-implemented validation for external source record limitsThis function properly implements the business logic to restrict records in external sources:
- Checks if the requested page exceeds the 100 record threshold
- Shows an upgrade modal if needed
- Only calls the callback if validation passes
The conditional logic is clear and follows the "early return" pattern for failed validation.
253-253: Fix for pagination boundary conditionThe modified condition correctly uses
(pageSize ?? 10) - 1instead of directly comparing withpageSize, fixing an off-by-one error in pagination boundary checks.
256-260: Integration of validation in navigation flowThe external source validation is well integrated into the navigation flow. If validation fails:
- The expanded form loading is stopped via
stopLoading()- The function returns early, preventing navigation to pages beyond the limit
This ensures a consistent user experience when limits are reached.
324-324: Validation wrapper for page change callbackThe
change-pageprop now correctly wraps the original callback with validation, maintaining the same pattern used ingoToNextRow. This ensures consistent validation across all pagination entry points.
355-355: Reference binding for expanded form componentAdding the ref binding to
expandedFormRefcorrectly connects the template reference to the reactive reference declared earlier, enabling access to component methods likestopLoading.
|
Uffizzi Preview |
56fd565 to
ce24c41
Compare
ce24c41 to
ceea7b6
Compare
Change Summary
Provide summary of changes with issue number if any.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of