-
-
Notifications
You must be signed in to change notification settings - Fork 18
Add ordering and ordering_field to AIGeneratedTableSettings for enhanced table sorting #1527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ced table sorting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the AI-generated table settings by adding support for default table sorting configuration. The AI service now generates ordering (ASC/DESC) and ordering_field (column name) properties for each table, allowing tables to have sensible default sort orders.
Changes:
- Added
orderingandordering_fieldfields to theAIGeneratedTableSettingsinterface - Updated the AI prompt to instruct the model to generate appropriate sorting configuration
- Implemented validation and mapping logic to process AI-generated sorting values
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| settings.readonly_fields = this.filterValidColumns(tableSettings.readonly_fields, validColumnNames); | ||
| settings.columns_view = this.filterValidColumns(tableSettings.columns_view, validColumnNames); | ||
| settings.ordering = this.mapOrdering(tableSettings.ordering); | ||
| settings.ordering_field = validColumnNames.includes(tableSettings.ordering_field) ? tableSettings.ordering_field : null; |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic doesn't handle the case when tableSettings.ordering_field is undefined or null. If the AI response is missing this field or it's null, validColumnNames.includes(undefined) or validColumnNames.includes(null) will be called, which will always return false but is not semantically clear. Consider adding an explicit check for undefined/null before the includes check.
| settings.ordering_field = validColumnNames.includes(tableSettings.ordering_field) ? tableSettings.ordering_field : null; | |
| const orderingField = tableSettings.ordering_field; | |
| settings.ordering_field = | |
| orderingField != null && validColumnNames.includes(orderingField) ? orderingField : null; |
| settings.ordering = this.mapOrdering(tableSettings.ordering); | ||
| settings.ordering_field = validColumnNames.includes(tableSettings.ordering_field) ? tableSettings.ordering_field : null; |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no validation to ensure that ordering and ordering_field are set together. If the AI provides an ordering direction but an invalid ordering_field (or vice versa), the table settings will have incomplete sorting configuration. Consider validating that if one is set, the other should also be set, or set both to null if either is invalid to maintain consistency.
| settings.ordering = this.mapOrdering(tableSettings.ordering); | |
| settings.ordering_field = validColumnNames.includes(tableSettings.ordering_field) ? tableSettings.ordering_field : null; | |
| const mappedOrdering = this.mapOrdering(tableSettings.ordering); | |
| const validOrderingField = validColumnNames.includes(tableSettings.ordering_field) | |
| ? tableSettings.ordering_field | |
| : null; | |
| if (mappedOrdering && validOrderingField) { | |
| settings.ordering = mappedOrdering; | |
| settings.ordering_field = validOrderingField; | |
| } else { | |
| settings.ordering = null; | |
| settings.ordering_field = null; | |
| } |
No description provided.