refactor: Decouple users in role table data from layout #37184
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37184 +/- ##
===========================================
+ Coverage 66.00% 67.54% +1.54%
===========================================
Files 3014 3295 +281
Lines 108199 112648 +4449
Branches 19476 20454 +978
===========================================
+ Hits 71417 76091 +4674
+ Misses 34402 33881 -521
- Partials 2380 2676 +296
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughPropagates unified pagination state ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin User
participant Page as UsersInRolePage
participant Pag as usePagination
participant Query as useQuery(getUsersInRole)
participant API as /v1/roles.getUsersInRole
Admin->>Page: Open "Users in Role" page
Page->>Pag: init(current, itemsPerPage)
Page->>Query: fetch({ role, scope, count, offset })
Query->>API: GET users { role, scope, count, offset }
API-->>Query: { users, total }
Query-->>Page: data, isLoading/isError/isSuccess
Page->>Admin: Render UsersInRoleTable(users, total, paginationData)
Admin->>Pag: Change page or per-page
Pag-->>Page: update current/itemsPerPage
Page->>Query: refetch with new count/offset
sequenceDiagram
autonumber
actor Admin as Admin User
participant Table as UsersInRoleTable
participant Hook as useRemoveUserFromRole
participant Modal as GenericModal
participant API as /v1/roles.removeUserFromRole
participant QC as QueryClient
Admin->>Table: Click "Remove" on a user
Table->>Hook: handleRemove(username)
Hook->>Modal: open confirm (danger)
Admin->>Modal: Confirm
Modal->>API: POST removeUserFromRole { roleId, username, scope }
API-->>Modal: 200 OK
Modal->>QC: invalidate getUsersInRole
Modal->>Hook: close modal
Hook->>Table: (toast shown) refetch()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
dougfabris
force-pushed
the
refactor/usersInRole-table
branch
2 times, most recently
from
3565f34 to
d90d92a
Compare
dougfabris
force-pushed
the
refactor/usersInRole-table
branch
from
d90d92a to
ba85c24
Compare
dougfabris
force-pushed
the
refactor/usersInRole-table
branch
from
ba85c24 to
9b04d42
Compare
dougfabris
changed the title
![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
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.tsx (1)
50-69: Consider removing unnecessary optional chaining on the map key.The rendering logic and pagination integration are correct. However, on line 56,
user?._iduses optional chaining even though theuserstype isSerialized<IUserInRole>[], which doesn't permit undefined elements. This suggests either:
- The optional chaining is defensive but unnecessary, or
- There's a type mismatch where users could contain undefined elements
If the type accurately reflects the data, consider simplifying to
user._idfor consistency with the type contract.Apply this diff if the type contract is accurate:
- {users.map((user) => ( - <UsersInRoleTableRow key={user?._id} user={user} onRemove={onRemove} /> - ))} + {users.map((user) => ( + <UsersInRoleTableRow key={user._id} user={user} onRemove={onRemove} /> + ))}
📜 Review details
Configuration used: CodeRabbit 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 ignored due to path filters (2)
apps/meteor/client/views/admin/permissions/PermissionsTable/__snapshots__/PermissionsTable.spec.tsx.snapis excluded by!**/*.snapapps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/__snapshots__/UsersInRoleTable.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsPage.tsx(2 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx(3 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx(2 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx(2 hunks)apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRolePage.tsx(4 hunks)apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.spec.tsx(1 hunks)apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx(1 hunks)apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.tsx(4 hunks)apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTableRow.tsx(2 hunks)apps/meteor/client/views/admin/permissions/UsersInRole/hooks/useRemoveUserFromRole.tsx(1 hunks)apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.spec.tsx
🧬 Code graph analysis (11)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.tsx (1)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(20-49)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx (1)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
createMockedPagination(8-15)
apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (1)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
createMockedPagination(8-15)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx (3)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx (2)
Default(36-36)Empty(60-60)apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (1)
Default(13-13)apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
createMockedPagination(8-15)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.tsx (2)
packages/core-typings/src/IUser.ts (1)
IUserInRole(295-298)apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(20-49)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx (3)
packages/apps-engine/src/server/permissions/AppPermissions.ts (1)
defaultPermissions(133-164)apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
createMockedPagination(8-15)packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRolePage.tsx (2)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(20-49)apps/meteor/client/views/admin/permissions/UsersInRole/hooks/useRemoveUserFromRole.tsx (1)
useRemoveUserFromRole(8-52)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTableRow.tsx (1)
packages/core-typings/src/IUser.ts (1)
IUserInRole(295-298)
apps/meteor/client/views/admin/permissions/UsersInRole/hooks/useRemoveUserFromRole.tsx (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/ui-contexts/src/index.ts (2)
useSetModal(68-68)useToastMessageDispatch(75-75)packages/core-typings/src/IUser.ts (1)
IUserInRole(295-298)
apps/meteor/client/views/admin/permissions/PermissionsPage.tsx (2)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(20-49)apps/meteor/client/views/admin/permissions/hooks/usePermissionsAndRoles.ts (1)
usePermissionsAndRoles(10-35)
🪛 Biome (2.1.2)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx
[error] 72-72: Do not shadow the global "Error" 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)
🔇 Additional comments (9)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx (3)
6-6: LGTM!The import is correctly added and properly scoped for the mocked pagination helper.
102-111: LGTM! Template pattern and realistic pagination testing.The Template pattern follows Storybook best practices. The configuration with
total: 10andpermissions.length(4 items) creates a realistic pagination scenario where the UI displays "Showing results 1 - 4 of 10", effectively testing the pagination component behavior when there are more total items than currently displayed.
113-120: LGTM!The Empty story correctly configures the empty state with appropriate zero-count pagination data.
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.tsx (6)
1-1: LGTM!The addition of
IUserInRoleandSerializedtypes properly supports the refactored props interface.
15-15: LGTM!The type import for
usePaginationcorrectly supports thepaginationDataprop typing.
17-26: LGTM! Excellent separation of concerns.The refactored props interface properly decouples data fetching from presentation. The component is now purely presentational, receiving all state and callbacks externally. The use of
Serialized<IUserInRole>[]correctly handles API response data types.
30-30: LGTM!The destructuring pattern correctly extracts specific pagination properties while spreading the remaining helpers (
itemsPerPageLabel,showingResultsLabel) to thePaginationcomponent.
32-48: LGTM!The header structure correctly includes three columns (Name, Email, Actions), and the loading state properly reflects this with
headerCells={3}. The fixed width for the Actions column (w='x80') provides appropriate sizing for action buttons.
71-72: LGTM! Proper empty and error state handling.The component correctly displays
GenericNoResultsfor empty datasets andGenericErrorwith a retry callback for error states. Note thatusers?.length(line 71) uses optional chaining consistent with line 50, though the type contract suggestsusersis always defined (non-optional prop).
...eor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx
Outdated
Show resolved
Hide resolved
MartinSchoeler
left a comment
MartinSchoeler
left a comment
There was a problem hiding this comment.
let's move this to a dedicated file
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
8-10: LGTM! Consider making the TODO more actionable.The TODO appropriately tracks technical debt for relocating this hook, given its broader usage across the codebase.
To improve clarity, consider specifying a destination folder (e.g.,
client/hooks/orclient/lib/hooks/) or linking to a tracking issue:/** - * TODO: Move `usePagination` outside from `GenericTable` folder + * TODO: Move `usePagination` to `client/hooks/` for broader reusability */apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx (1)
48-82: Stories correctly demonstrate different UI states; consider consistent naming.The three stories properly configure state flags and data for loading, no results, and error scenarios. The previous shadowing issue with the
Errorexport has been resolved.However, consider using PascalCase for all story exports to maintain consistency within the file and align with the codebase convention observed in
PermissionsTable.stories.tsxandUsersTable.stories.tsx.Apply this diff to align naming conventions:
-export const withLoading = Template.bind({}); -withLoading.args = { +export const WithLoading = Template.bind({}); +WithLoading.args = { total: 0, isLoading: true, isError: false, isSuccess: false, users: [], onRemove: () => undefined, refetch: () => undefined, paginationData: createMockedPagination(), }; -export const withNoResults = Template.bind({}); -withNoResults.args = { +export const WithNoResults = Template.bind({}); +WithNoResults.args = { total: 0, isLoading: false, isError: false, isSuccess: true, users: [], onRemove: () => undefined, refetch: () => undefined, paginationData: createMockedPagination(), }; -export const withError = Template.bind({}); -withError.args = { +export const WithError = Template.bind({}); +WithError.args = { total: 0, isLoading: false, isError: true, isSuccess: false, users: [], onRemove: () => undefined, refetch: () => undefined, paginationData: createMockedPagination(), };
📜 Review details
Configuration used: CodeRabbit 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 ignored due to path filters (1)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/__snapshots__/UsersInRoleTable.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx(3 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx(2 hunks)apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx(1 hunks)apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx (2)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx (1)
Default(36-36)apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (1)
Default(13-13)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx (2)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx (1)
Default(104-104)apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (1)
Default(13-13)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (10)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx (3)
1-17: LGTM! Well-structured story setup.The imports, meta configuration, and decorator setup follow Storybook best practices. The
satisfieskeyword ensures type safety, and the PageContent decorator provides consistent layout across all stories.
19-32: LGTM! Mock data generator works correctly.The
generateMockedUsersfunction appropriately creates test data with varied properties. The roles assignment logic ensures the first 5 users receive admin roles, which aligns with the current usage.
34-46: LGTM! Default story demonstrates pagination correctly.The Template and Default story implementation is clean. The pagination setup (5 users displayed out of 30 total) effectively demonstrates the table's pagination behavior.
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx (3)
10-10: LGTM!The import of
createMockedPaginationcorrectly provides the mock pagination data needed for the updated component API.
54-65: LGTM!The test correctly adapts to the new
paginationDataprop while maintaining its original logic. The multi-line formatting improves readability.
80-91: LGTM!The test update is consistent with the first test and correctly provides the required
paginationDataprop.apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx (4)
6-6: LGTM!The import of
createMockedPaginationis necessary for the pagination refactor and consistent with the test file.
102-103: LGTM!The Template pattern is a Storybook best practice and aligns with patterns used elsewhere in the codebase.
104-111: Verify thetotalandpermissions.lengthmismatch.The
permissionsarray contains 4 items, buttotalis set to 10, andcreateMockedPagination(permissions.length, 10)is called with arguments4and10.This could be intentional to demonstrate pagination (showing page 1 with 4 items from a total of 10), but it creates an inconsistency where the rendered data (4 permissions) doesn't match the declared total (10).
Please confirm whether this is intentional or if
totalshould bepermissions.lengthfor internal consistency.
113-120: LGTM!The Empty story correctly adopts the Template pattern and maintains internal consistency with
total: 0and empty data arrays.
dougfabris
force-pushed
the
refactor/usersInRole-table
branch
from
859ce49 to
a8a3062
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx (1)
72-82: Past issue resolved: export no longer shadows globalError.The story is now correctly named
withErrorinstead ofError, resolving thenoShadowRestrictedNameslint violation from the previous review.
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx (1)
1-82: LGTM! Well-structured story file with consistent patterns.The new story file follows Storybook best practices:
- Template pattern for story reuse
- Consistent use of
createMockedPaginationacross all stories- Clear separation of loading, error, and success states
- Local
generateMockedUsershelper appropriately scoped to this fileIf the
generateMockedUsershelper is needed in other test files, consider moving it totests/mocks/data.tsalongsidecreateMockedPaginationto reduce duplication.
📜 Review details
Configuration used: CodeRabbit 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 ignored due to path filters (1)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/__snapshots__/UsersInRoleTable.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts(1 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx(3 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx(2 hunks)apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx(1 hunks)apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx(3 hunks)apps/meteor/tests/mocks/data.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx
- apps/meteor/client/components/GenericTable/hooks/usePagination.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (1)
apps/meteor/tests/mocks/data.ts (1)
createMockedPagination(457-464)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx (1)
apps/meteor/tests/mocks/data.ts (1)
createMockedPagination(457-464)
apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx (1)
apps/meteor/tests/mocks/data.ts (1)
createMockedPagination(457-464)
⏰ 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). (3)
- GitHub Check: CodeQL-Build
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (1)
5-5: LGTM! Consistent use of shared pagination mock.The migration to
createMockedPaginationimproves consistency across story files and centralizes the pagination mock logic.Also applies to: 71-71, 77-77, 87-87
apps/meteor/tests/mocks/data.ts (1)
457-464: LGTM! Well-designed pagination mock helper.The
createMockedPaginationutility provides a clean abstraction for pagination state in tests and stories, improving maintainability across the codebase.apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx (1)
6-6: LGTM! Proper Template pattern and pagination mock usage.The introduction of the Template pattern and migration to
createMockedPaginationaligns with Storybook best practices and maintains consistency across the codebase.Also applies to: 102-120
dougfabris
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit 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)
apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx(4 hunks)apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx(2 hunks)apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx(1 hunks)apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.spec.tsx
- apps/meteor/client/views/admin/permissions/PermissionsTable/PermissionsTable.stories.tsx
- apps/meteor/client/views/admin/permissions/UsersInRole/UsersInRoleTable/UsersInRoleTable.stories.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (1)
apps/meteor/tests/mocks/data.ts (1)
createMockedPagination(457-464)
🔇 Additional comments (1)
apps/meteor/client/views/admin/users/UsersTable/UsersTable.stories.tsx (1)
13-64: LGTM! Well-structured mock data.The mock users array provides good coverage of different user states, statuses, and role combinations for visual testing.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
SUP-856
Summary by CodeRabbit