feat(federation): Added room license and settings validations #6859
Conversation
aleksandernsilva
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Warning Rate limit exceeded@diegolmello has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (26)
WalkthroughAdds typed federation data to room/subscription models, DB schema/migration and model serialization, new default settings and localization keys, federation helpers/type guards, merge propagation, and RoomView UI changes to display federation footer descriptions. Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are primarily additions of fields, types, settings, localization, and localized UI rendering without a complex multi-component sequential control flow that requires visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 review |
✅ Actions performedReview triggered.
|
aleksandernsilva
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
aleksandernsilva
had a problem deploying
to
official_android_build
GitHub Actions
Error
aleksandernsilva
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
aleksandernsilva
had a problem deploying
to
official_ios_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/i18n/locales/en.json (1)
354-355: Minor grammar improvement suggested.The localization keys are well-named and the first message is clear. However, the second message has a minor grammatical issue: "Join federated rooms" should be "Joining federated rooms" to maintain parallel structure with other feature descriptions.
🔎 Consider this grammar improvement:
- "Federation_Matrix_room_description_missing_module": "Join federated rooms is a Premium feature", + "Federation_Matrix_room_description_missing_module": "Joining federated rooms is a Premium feature",app/views/RoomView/index.tsx (2)
105-105: Import is correct, but underlying helper has typing concern.The import of
isRoomFederatedis appropriate for the federation feature. However, the helper implementation uses(room as any).federatedwhich suggests the type system doesn't properly account for thefederatedproperty on room objects. This is pre-existing technical debt but worth noting.Consider updating the room types to properly include the
federatedproperty to eliminate the need for type assertions inisRoomFederated.
1671-1673: Federation state mapping logic is correct, but consider defensive checks.The OR logic on line 1672 correctly implements the requirement: federation is enabled if either
Federation_Matrix_enabledorFederation_Service_Enabledis true. This aligns with the PR description stating that both must be disabled to show the "disabled" message.However, consider adding explicit boolean coercion or handling for cases where these settings might be undefined or null:
🔎 Consider more defensive boolean coercion:
encryptionEnabled: state.encryption.enabled, - isFederationEnabled: (state.settings.Federation_Matrix_enabled || state.settings.Federation_Service_Enabled) as boolean, + isFederationEnabled: Boolean(state.settings.Federation_Matrix_enabled || state.settings.Federation_Service_Enabled), - isFederationModuleEnabled: state.enterpriseModules.includes('federation') as boolean + isFederationModuleEnabled: Boolean(state.enterpriseModules?.includes('federation'))This provides clearer intent and handles potential undefined/null values more safely.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/i18n/locales/en.json(1 hunks)app/lib/constants/defaultSettings.ts(1 hunks)app/views/RoomView/definitions.ts(1 hunks)app/views/RoomView/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/RoomView/index.tsx (2)
app/lib/methods/isRoomFederated.ts (1)
isRoomFederated(7-8)app/lib/constants/colors.ts (1)
themes(304-304)
⏰ 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: ESLint and Test / run-eslint-and-test
🔇 Additional comments (4)
app/views/RoomView/definitions.ts (1)
35-36: LGTM! Clear federation-related props added.The addition of
isFederationEnabledandisFederationModuleEnabledboolean properties toIRoomViewPropsis clean and follows TypeScript best practices. These props provide clear separation between the federation service being enabled and the federation module/license being available.app/lib/constants/defaultSettings.ts (1)
303-308: LGTM! Federation settings added correctly.The two new federation-related settings follow the established pattern for boolean settings in the codebase. The placement before
deprecatedSettingsis appropriate, and the naming convention is consistent with other settings in the file.app/views/RoomView/index.tsx (2)
1339-1351: LGTM! Clear federation status logic.The
getFederatedFooterDescriptionmethod correctly prioritizes checking if federation is enabled before checking module availability. The logic flow is clean:
- First checks if federation service is disabled → returns disabled message
- Then checks if federation module/license is missing → returns premium feature message
- Otherwise returns
undefinedto allow normal operationThe method is pure, has clear return types, and uses proper I18n localization.
1523-1533: LGTM! Federation footer rendering is well-integrated.The federation footer rendering follows the established pattern for other room states (blocked, read-only, on-hold). The checks are appropriate:
- Type guard
'id' in roomensures room has necessary propertiesisRoomFederated(room)determines if federation logic should apply- Conditional rendering based on
descriptionpresence prevents unnecessary rendersThe styling and placement are consistent with other footer states in the same method.
aleksandernsilva
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/lib/database/model/Subscription.js (1)
156-156: LGTM: Federation field added with proper decorator.The
@json('federation', sanitizer)decorator correctly handles JSON serialization of the federation object, following the same pattern used for other structured fields likeroles,muted, andvisitor.Note: The
sanitizerfunction (fromapp/lib/database/utils.ts) is a passthrough that doesn't validate the federation object structure. If stricter validation is needed in the future, consider creating a dedicated sanitizer that validates the{ version, mrid, origin }shape.app/lib/methods/helpers/mergeSubscriptionsRooms.ts (1)
17-17: Acceptable: Complexity directive added.The ESLint complexity directive is added, likely due to the growing number of conditional checks in the
mergefunction. While this is acceptable for now, consider refactoring the function into smaller, focused helpers if complexity continues to increase.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
app/definitions/IRoom.tsapp/definitions/ISubscription.tsapp/i18n/locales/en.jsonapp/lib/database/model/Subscription.jsapp/lib/database/model/migrations.jsapp/lib/database/schema/app.jsapp/lib/methods/helpers/mergeSubscriptionsRooms.tsapp/lib/methods/isRoomFederated.tsapp/views/RoomView/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/RoomView/index.tsx
- app/i18n/locales/en.json
🧰 Additional context used
🧬 Code graph analysis (2)
app/lib/database/model/Subscription.js (1)
app/lib/database/utils.ts (1)
sanitizer(19-19)
app/lib/methods/isRoomFederated.ts (1)
app/definitions/ISubscription.ts (1)
ISubscription(40-124)
🔇 Additional comments (10)
app/lib/database/schema/app.js (2)
4-4: LGTM: Schema version bumped correctly.The schema version increment from 27 to 28 follows the sequential migration pattern and aligns with the new migration step added in
app/lib/database/model/migrations.js.
73-74: LGTM: Federation columns added to subscriptions.The addition of both
federated(boolean) andfederation(string) columns is consistent with the migration and type definitions. The string type forfederationenables storing JSON-serialized federation metadata (version, mrid, origin).app/lib/database/model/migrations.js (1)
335-343: LGTM: Migration for federation column is properly structured.The version 28 migration correctly adds the
federationcolumn to the subscriptions table, matching the schema definition inapp/lib/database/schema/app.js. The migration follows the established pattern and is properly positioned after the version 27 migration.app/lib/database/model/Subscription.js (1)
224-225: LGTM: Federation included in serialization.The federation field is correctly included in the
asPlain()method, ensuring proper serialization of subscription objects.app/lib/methods/helpers/mergeSubscriptionsRooms.ts (1)
107-109: LGTM: Federation data propagated correctly.The federation field is properly propagated from the room object to the merged subscription, following the same pattern as other optional properties like
usersCount. The'federation' in roomcheck ensures safe property access.app/definitions/ISubscription.ts (1)
119-123: LGTM: Federation field properly typed.The
federationfield is correctly defined as an optional structured object withversion,mrid, andoriginproperties. This matches the federation type added toIServerRoominapp/definitions/IRoom.ts(lines 166-170), ensuring consistency across room and subscription interfaces.app/definitions/IRoom.ts (1)
166-170: LGTM: Federation type narrowed for better type safety.The
federationfield type has been improved fromanyto a properly structured object withversion,mrid, andoriginproperties. This change:
- Matches the
ISubscription.federationtype (lines 119-123 inapp/definitions/ISubscription.ts)- Provides better type safety and IDE autocomplete
- Enables compile-time validation of federation metadata
app/lib/methods/isRoomFederated.ts (3)
3-5: LGTM: IRoomFederated exported for external use.The
exportkeyword correctly exposes theIRoomFederatedinterface, enabling its use in RoomView and other components that need to work with federated rooms.
7-13: LGTM: IRoomNativeFederated interface properly defined.The new
IRoomNativeFederatedinterface extendsIRoomFederatedand adds the requiredfederationobject withversion,mrid, andoriginproperties. This allows distinguishing between:
- Rooms that are simply marked as federated (
federated: true)- Rooms with full federation metadata (native federated rooms)
The federation object structure matches the types defined in
ISubscriptionandIServerRoom.
18-19: LGTM: Type guard correctly narrows to IRoomNativeFederated.The
isRoomNativeFederatedtype guard properly checks:
- The room passes the
isRoomFederatedcheck- The
federationproperty exists- The federation object is truthy
This ensures safe narrowing to the
IRoomNativeFederatedtype.
diegolmello
changed the title
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/views/RoomView/index.tsx (1)
1702-1703: Verify the federation enabled logic and handle undefined settings.Two concerns with the state mapping:
OR logic verification: Line 1702 enables federation if EITHER
Federation_Matrix_enabledORFederation_Service_Enabledis true. Please confirm this is the intended behavior. Should both settings need to be enabled instead?Type safety: Settings might be
undefinedif not configured. The current implementation with||andas booleancasts could hide undefined values. Consider explicit handling:🔎 Suggested improvement for type safety
- isFederationEnabled: (state.settings.Federation_Matrix_enabled || state.settings.Federation_Service_Enabled) as boolean, - isFederationModuleEnabled: state.enterpriseModules.includes('federation') as boolean + isFederationEnabled: !!(state.settings.Federation_Matrix_enabled || state.settings.Federation_Service_Enabled), + isFederationModuleEnabled: state.enterpriseModules?.includes('federation') ?? falsePlease also confirm that
isFederationEnabledandisFederationModuleEnabledhave been added to theIRoomViewPropsinterface definition, as mentioned in the PR description but not visible in the provided diff.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/definitions/ISubscription.tsapp/i18n/locales/en.jsonapp/lib/database/model/Subscription.jsapp/lib/database/model/migrations.jsapp/lib/database/schema/app.jsapp/lib/methods/helpers/mergeSubscriptionsRooms.tsapp/views/RoomView/index.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- app/lib/database/schema/app.js
- app/lib/database/model/Subscription.js
- app/i18n/locales/en.json
- app/lib/methods/helpers/mergeSubscriptionsRooms.ts
- app/lib/database/model/migrations.js
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/RoomView/index.tsx (1)
app/lib/methods/isRoomFederated.ts (3)
IRoomFederated(3-5)isRoomNativeFederated(18-19)isRoomFederated(15-16)
🔇 Additional comments (6)
app/definitions/ISubscription.ts (1)
119-123: LGTM! Clean addition of federation metadata.The optional
federationobject is well-typed and appropriately structured for federation support. The optional nature correctly reflects that not all subscriptions are federated.app/views/RoomView/index.tsx (5)
105-105: LGTM! Appropriate federation helper imports.The imports provide necessary type guards and types for federation feature support.
1354-1370: LGTM! Well-structured federation description logic.The method correctly handles the cascade of federation checks:
- Validates native federation support (presence of
federationobject)- Checks if federation is enabled via settings
- Verifies enterprise module availability
Returns appropriate localized messages for each failure scenario.
1388-1388: LGTM! Proper federation detection for room.The type guard and federation check correctly identify federated rooms for conditional rendering logic.
1456-1456: LGTM! Read receipts correctly disabled for federated rooms.Disabling read receipts for federated rooms is appropriate as this feature may not be supported across federation boundaries.
1542-1554: LGTM! Federation footer rendering follows established patterns.The implementation correctly:
- Type-checks the room before calling federation helpers
- Conditionally renders only when a description exists
- Follows the same read-only footer pattern used elsewhere in the method
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
Proposed changes
This pull request adds support for displaying appropriate messages in the room footer when federation features are disabled or unavailable. It introduces new settings, props, and logic to determine the federation status and show relevant descriptions to the user in the
RoomViewcomponent.Federation feature support and UI updates:
Federation_Service_EnabledandFederation_Matrix_enabled, to thedefaultSettingsconstant.RoomViewto provideisFederationEnabledandisFederationModuleEnabledprops, based on the new settings and available enterprise modules.IRoomViewPropsinterface to include the new federation-related props.RoomViewto determine and display a footer description if federation is disabled or the required module is missing, using new i18n strings.Issue(s)
FB-74
How to test or reproduce
Federation_Matrix_enabledandFederation_Service_Enabledare disabled.federation)Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.