Conversation
- exposed types and constants required in other packages - fixed migrations - fixed some antd warnings
WalkthroughRemoves a domain configuration class, applies null-safe handling for notification Category/OverrideChannels, adds foreign-key relocation steps in multiple migrations, tightens and expands React component props and public exports, and removes an unused using directive and a deleted no-op migration file. Changes
Sequence Diagram(s)(Skipped — changes are localized logic/migration and API surface edits; no new/high-level control flow to visualize.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
⏰ 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)
🔇 Additional comments (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shesha-core/src/Shesha.Core/Domain/RoleAppointmentTypeConfig.cs (1)
6-35: Delete the commented-out class entirely rather than leaving it in the codebase.Verification confirms this is an intentional removal: migrations
M20250508144200.cs(creates table) andM20250623120299.cs(deletes table and foreign key) properly handle the database-side deprecation. No other code references this class, so removing it won't cause issues.However, commented-out code should not be committed. Since the database migrations manage the removal and no active code depends on this class, delete
shesha-core/src/Shesha.Core/Domain/RoleAppointmentTypeConfig.csentirely. Git history preserves the implementation if needed later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
shesha-core/src/Shesha.Application/Notifications/Distribution/NotificationTypes/NotificationTypeImport.cs(1 hunks)shesha-core/src/Shesha.Application/Notifications/NotificationExtensions.cs(1 hunks)shesha-core/src/Shesha.Core/Domain/RoleAppointmentTypeConfig.cs(1 hunks)shesha-core/src/Shesha.Framework/Dto/ConfigurationItemDto.cs(0 hunks)shesha-core/src/Shesha.Framework/Migrations/ConfigurationStudio/M20250623120299.cs(1 hunks)shesha-core/src/Shesha.Framework/Migrations/ConfigurationStudio/M20250623120499.cs(1 hunks)shesha-core/src/Shesha.Framework/Migrations/ConfigurationStudio/M20250912170999.cs(1 hunks)shesha-core/src/Shesha.Framework/Migrations/M20250529124100.cs(0 hunks)shesha-reactjs/src/components/index.ts(2 hunks)shesha-reactjs/src/components/refListDropDown/genericRefListDropDown.tsx(4 hunks)shesha-reactjs/src/components/refListDropDown/models.ts(1 hunks)shesha-reactjs/src/index.tsx(1 hunks)shesha-reactjs/src/interfaces/index.ts(3 hunks)shesha-reactjs/src/utils/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- shesha-core/src/Shesha.Framework/Migrations/M20250529124100.cs
- shesha-core/src/Shesha.Framework/Dto/ConfigurationItemDto.cs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-06T10:09:19.065Z
Learnt from: Tshepo1103-lab
Repo: shesha-io/shesha-framework PR: 4013
File: shesha-core/src/Shesha.NHibernate/Migrations/M20251106095300.cs:29-51
Timestamp: 2025-11-06T10:09:19.065Z
Learning: The legacy table Frwk_StoredFileVersionDownloads in the Shesha framework did not have a TenantId column. When migrating to the new frwk.stored_file_version_downloads table in migration M20251106095300, setting tenant_id to NULL for historical records is correct.
Applied to files:
shesha-core/src/Shesha.Framework/Migrations/ConfigurationStudio/M20250623120299.csshesha-core/src/Shesha.Framework/Migrations/ConfigurationStudio/M20250912170999.csshesha-core/src/Shesha.Framework/Migrations/ConfigurationStudio/M20250623120499.cs
📚 Learning: 2025-11-20T14:51:46.128Z
Learnt from: AlexStepantsov
Repo: shesha-io/shesha-framework PR: 4235
File: shesha-reactjs/src/components/modelConfigurator/propertiesEditor/provider/reducer.ts:149-149
Timestamp: 2025-11-20T14:51:46.128Z
Learning: In shesha-reactjs model configurator reducer (src/components/modelConfigurator/propertiesEditor/provider/reducer.ts), when handling ArrayFormats.childObjects in the UpdateItem action, itemsType.dataFormat should be set to undefined (not ObjectFormats.object). This is the correct logic for child object array configuration.
Applied to files:
shesha-reactjs/src/components/refListDropDown/models.tsshesha-reactjs/src/components/index.ts
📚 Learning: 2025-07-19T10:27:44.987Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 0
File: :0-0
Timestamp: 2025-07-19T10:27:44.987Z
Learning: In shesha-reactjs code editor settings, removing the "availableConstantsExpression" property forces the code editor to expose the default list of exposed variables instead of a custom subset. This is useful when you want to provide access to the full range of available variables rather than maintaining a custom limited list.
Applied to files:
shesha-reactjs/src/components/index.ts
📚 Learning: 2025-06-26T20:00:58.574Z
Learnt from: teboho
Repo: shesha-io/shesha-framework PR: 3461
File: shesha-reactjs/src/designer-components/charts/chartControl.tsx:51-51
Timestamp: 2025-06-26T20:00:58.574Z
Learning: In shesha-reactjs chart components (ChartControl), filters are intentionally passed as props rather than obtained from context. This allows the query builder and other external components to dynamically update filters. The pattern is: most chart configuration comes from useChartDataStateContext(), but filters are passed as props from the chart factories (bar.tsx, line.tsx, pie.tsx, polarArea.tsx) for dynamic updates.
Applied to files:
shesha-reactjs/src/interfaces/index.ts
⏰ 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: build-attempt
🔇 Additional comments (19)
shesha-core/src/Shesha.Application/Notifications/NotificationExtensions.cs (1)
16-17: LGTM! Good defensive null handling.The null-coalescing operators normalize null values to empty strings, ensuring consistent data representation and preventing potential null reference issues downstream.
shesha-core/src/Shesha.Framework/Migrations/ConfigurationStudio/M20250623120299.cs (1)
85-86: Verify data migration and FK handling before table drop.The
MoveForeignKeyscall on line 86 relocates foreign key constraints fromFrwk_FormConfigurationstoform_configuration_revisionsbefore dropping the source table. However, there is no visibleExecuteCsScriptcall to copy data fromFrwk_FormConfigurationstoform_configuration_revisionsin this migration segment, unlike other table migrations in the file (lines 47-49, 74).Additionally, the explicit FK drop on lines 80-84 is conditional, while
MoveForeignKeyson line 86 is unconditional. IfMoveForeignKeysattempts to move an already-dropped FK, it may fail.Verify:
- Has data from
Frwk_FormConfigurationsbeen copied toform_configuration_revisionsin this or a prior migration?- Does
MoveForeignKeysgracefully handle FKs that have already been dropped?- Are all FKs referencing
Frwk_FormConfigurations.Idaccounted for in this migration?shesha-reactjs/src/utils/index.ts (1)
174-175: LGTM! Public API expansion aligns with PR objectives.The addition of datatable and URL utility re-exports successfully expands the public API surface, making these utilities accessible to external consumers.
shesha-reactjs/src/components/refListDropDown/genericRefListDropDown.tsx (3)
1-1: LGTM! Type safety improvement.Adding the
SelectPropsimport enables proper typing for thecommonSelectPropsobject, improving type safety throughout the component.
33-36: LGTM! Public API expansion aligns with PR objectives.The addition of
size,variant,className, anddefaultValueprops expands the component's public API surface, giving consumers more control over the rendered Select component.
131-152: LGTM! Explicit prop assignment improves maintainability.The refactor from spreading
...restto explicitly assigning props makes the component's API surface more predictable and maintainable. The addition ofPartial<SelectProps>typing further improves type safety.shesha-reactjs/src/interfaces/index.ts (3)
31-31: LGTM! DataTable interface exports expand public API.Adding
IStoredFilterandFilterExpressionexports enables external packages to work with DataTable filter types.
45-49: LGTM! Metadata and Ajax response exports expand public API.The addition of
IEntityReferencePropertyMetadata,isEntityReferencePropertyMetadata,NestedPropertyMetadatAccessor, and various Ajax response types/helpers provides external packages with necessary types and utilities for metadata handling and response processing.
62-63: LGTM! Settings and GraphQL exports expand public API.Adding
ISettingsComponent,ISettingsComponentGroup, and GraphQL re-exports provides external packages with settings configuration and GraphQL types.shesha-reactjs/src/components/index.ts (2)
90-90: LGTM! Code editor interface exports expand public API.Exporting
ICodeEditorProps,GetResultTypeFunc, andGetAvailableConstantsFuncenables external packages to work with code editor types and customize editor behavior.
112-112: LGTM! ListItem export expands public API.Exporting the
ListItemtype enables external packages to work with list editor data structures.shesha-reactjs/src/index.tsx (1)
34-35: LGTM! Form factory exports expand public API.Exporting
useFormViaFactory,useFormBuilderFactoryhooks andFormBuilder,FormBuilderFactorytypes enables external packages to leverage form factory functionality and type definitions.shesha-reactjs/src/components/refListDropDown/models.ts (1)
28-30: Request manual verification of breaking change impact.Automated searches for RefListDropDown component usages returned no results, preventing definitive verification of whether the API restriction (
PickvsOmit) breaks existing consumers.Please manually verify:
- Whether any code in the repository passes SelectProps outside the
LimitedSelectPropslist to this component- Whether this breaking change is intentional and documented
- Whether all necessary SelectProps are included in the
LimitedSelectPropslistThe concern remains valid—restricting from
Omit<SelectProps<any>, 'onChange'>toPick<SelectProps<any>, 'mode' | 'disabled' | ...>is a significant API change that could break consumers.shesha-core/src/Shesha.Framework/Migrations/ConfigurationStudio/M20250912170999.cs (4)
19-26: Verify foreign key handling for all tables being deleted.While
form_configuration_revisionsandrole_revisionshave their foreign keys explicitly moved (lines 16-17), the following six tables being deleted do not show correspondingMoveForeignKeysoperations:
- reference_list_revisions
- setting_config_revisions
- permission_definition_revisions
- notification_type_revisions
- notification_channel_revisions
- entity_config_revisions
Ensure these tables either have no foreign key constraints requiring relocation, or that FK handling is performed elsewhere in the migration sequence.
16-17: Foreign key relocation follows established pattern and is correctly ordered.The MoveForeignKeys operations on lines 16-17 properly relocate foreign keys before the corresponding table deletions on lines 19 and 22, consistent with similar migrations (M20250623120299.cs and M20250623120499.cs).
6-6: ****The migration number is correctly sequenced and will execute in the proper order. Analysis of the ConfigurationStudio migration directory shows that M20250912170999 is positioned chronologically between M20250910162099 (Sept 10) and M20250915174100 (Sept 15), with all subsequent migrations having higher numbers. Migration frameworks execute migrations in ascending numeric order, so this migration will run at the appropriate point regardless of when it was committed.
Likely an incorrect or invalid review comment.
11-12: ****The concern about NULL values is addressed by a preceding migration. The data migration is properly handled in M20250908141499.cs (Sept 8), which executes an UPDATE statement to populate NULL values in
entity_config_idbefore the NOT NULL constraint is applied in M20250912170999.cs (Sept 12). The migration sequence correctly follows the pattern: AddColumn(Nullable) → Execute.Sql UPDATE → AlterColumn(NotNullable).Likely an incorrect or invalid review comment.
shesha-core/src/Shesha.Framework/Migrations/ConfigurationStudio/M20250623120499.cs (2)
25-27: Unable to verify—manual review required.The migration file and related migrations could not be located in the codebase searches. This could indicate:
- The migration file is new to this PR and hasn't been committed to the base branch yet
- Target tables (
role_revisions,configuration_item_revisions,modules) may be created in separate migration contexts not detected by searchesVerify manually:
- Confirm that the target tables exist and are created in migrations that execute before M20250623120499
- Ensure the
MoveForeignKeysoperations will not fail due to missing target tables- Test the migration in a development environment before deployment
19-25: Verify FK recreation for manually dropped constraints.Lines 19-23 explicitly drop two foreign keys referencing
Core_ShaRolesbefore line 25 moves all remaining FKs torole_revisions.The
MoveForeignKeysmethod queries the database's system catalogs (sys.foreign_keys on SQL Server) to find FKs that reference the old table. Since the two constraints are already dropped, they won't be found by the query and therefore won't be recreated pointing torole_revisions.This means:
rpt_reportingReports.VisibilityRoleId→ loses its FK constraint (not migrated)entpr_DistributionListItems.ShaRoleId→ loses its FK constraint (not migrated)Verify whether this is intentional (removing role references entirely) or if these FKs should be manually recreated to point to
role_revisions.idafter theMoveForeignKeyscalls.
...rc/Shesha.Application/Notifications/Distribution/NotificationTypes/NotificationTypeImport.cs
Outdated
Show resolved
Hide resolved
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.