chore: new field for ABAC Attributes on user info #37135
Conversation
|
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 |
|
WalkthroughAdds ABAC attributes UI: new Tag and Attributes components, UserInfo accepts optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Browser
participant UserInfo as UserInfo component
participant InfoPanel as InfoPanel
participant I18N as i18n
participant AttrList as UserInfoABACAttributes
participant AttrTag as UserInfoABACAttribute
Browser->>UserInfo: mount(props { abacAttributes?, roles?, utcOffset? })
UserInfo->>InfoPanel: render base fields (name, username, etc.)
UserInfo->>I18N: t('ABAC_Attributes_description'), t('ABAC_Attributes')
alt abacAttributes present && abacAttributes.length > 0
UserInfo->>AttrList: render(abacAttributes[])
loop for each attribute
AttrList->>AttrTag: render(attribute)
end
else
UserInfo-->>InfoPanel: skip ABAC section
end
alt utcOffset is integer/truthy
UserInfo->>InfoPanel: render UTCClock in InfoPanelText
end
alt roles?.length > 0
UserInfo->>InfoPanel: render Roles block
else
UserInfo-->>InfoPanel: skip Roles block
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (5)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37135 +/- ##
===========================================
+ Coverage 67.44% 67.50% +0.06%
===========================================
Files 3289 3293 +4
Lines 111887 112426 +539
Branches 20442 20450 +8
===========================================
+ Hits 75463 75894 +431
- Misses 33741 33856 +115
+ Partials 2683 2676 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/meteor/client/components/UserCard/UserCardABACAttributes.tsx (1)
14-17: Consider using attribute value as key instead of index.Using array index as a key is an anti-pattern when list items can be reordered, added, or removed, as it can cause React reconciliation issues and performance problems. Since ABAC attributes are typically unique strings, consider using the attribute value itself as the key.
If attributes are guaranteed to be unique, apply this diff:
{abacAttributes.map((attribute, index) => ( - <Box m={2} fontScale='c2' key={index}> + <Box m={2} fontScale='c2' key={attribute}> <UserCardABACAttribute attribute={attribute} /> </Box> ))}If duplicate attributes are possible, use a composite key:
{abacAttributes.map((attribute, index) => ( - <Box m={2} fontScale='c2' key={index}> + <Box m={2} fontScale='c2' key={`${attribute}-${index}`}> <UserCardABACAttribute attribute={attribute} /> </Box> ))}apps/meteor/client/components/UserInfo/UserInfo.tsx (1)
193-193: Simplify to self-closing tag.The component has no children, so it can use a self-closing tag for cleaner syntax.
Apply this diff:
- <UserCardABACAttributes abacAttributes={abacAttributes}></UserCardABACAttributes> + <UserCardABACAttributes abacAttributes={abacAttributes} />
📜 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/components/UserCard/__snapshots__/UserCardABACAttributes.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
apps/meteor/client/components/UserCard/UserCardABACAttribute.tsx(1 hunks)apps/meteor/client/components/UserCard/UserCardABACAttributes.spec.tsx(1 hunks)apps/meteor/client/components/UserCard/UserCardABACAttributes.stories.tsx(1 hunks)apps/meteor/client/components/UserCard/UserCardABACAttributes.tsx(1 hunks)apps/meteor/client/components/UserInfo/UserInfo.tsx(4 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/components/UserCard/UserCardABACAttributes.stories.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/components/UserCard/UserCardABACAttributes.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
🪛 Biome (2.1.2)
apps/meteor/client/components/UserCard/UserCardABACAttribute.tsx
[error] 9-9: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
⏰ 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). (10)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (5/5) - Alpine (Official)
🔇 Additional comments (3)
apps/meteor/client/components/UserCard/UserCardABACAttributes.spec.tsx (1)
1-32: LGTM! Well-structured test coverage.The test file appropriately covers:
- Rendering with multiple attributes and snapshot testing
- Individual attribute presence verification
- Accessibility validation with jest-axe
The mock translations align with the component's i18n requirements.
apps/meteor/client/components/UserInfo/UserInfo.tsx (2)
74-75: LGTM! Appropriate temporary type suppression.The
@ts-expect-errorannotation with explanatory comment is the right approach for this UI-only implementation. The default value ofnulland the conditional rendering (line 181) properly handle the absence of the property until it's added to the User object.
185-191: No change needed; ‘status-font-on-info’ is the established accent for info icons and is used consistently across multiple components.Likely an incorrect or invalid review comment.
apps/meteor/client/components/UserCard/UserCardABACAttribute.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.stories.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/UserCard/UserCardABACAttributes.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.stories.tsx (1)
18-18: Translation key doesn't match UserInfo usage.The translation key
ABAC_attributes(lowercase 'a') doesn't match what UserInfo.tsx uses on line 184:ABAC_Attributes(capital 'A'). This will prevent the label from displaying correctly in Storybook.Apply this diff:
- ABAC_attributes: 'ABAC Attributes', + ABAC_Attributes: 'ABAC Attributes',
🧹 Nitpick comments (2)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx (1)
13-16: Consider stable keys if attributes can be reordered.Using array index as key is acceptable for static lists, but if
abacAttributescan be reordered or modified, consider using the attribute value itself as the key to maintain component identity.If attributes are unique, apply this diff:
- {abacAttributes.map((attribute, index) => ( - <Box m={2} fontScale='c2' key={index}> + {abacAttributes.map((attribute) => ( + <Box m={2} fontScale='c2' key={attribute}> <UserInfoABACAttribute attribute={attribute} /> </Box> ))}apps/meteor/client/components/UserInfo/UserInfo.tsx (1)
181-195: Simplify self-closing tag syntax.The component is rendered with an unnecessary self-closing tag pattern:
<UserInfoABACAttributes></UserInfoABACAttributes>. Since there are no children, use the simpler self-closing syntax.Apply this diff:
- <UserInfoABACAttributes abacAttributes={abacAttributes}></UserInfoABACAttributes> + <UserInfoABACAttributes abacAttributes={abacAttributes} />
📜 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/components/UserInfo/__snapshots__/UserInfoABACAttributes.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
apps/meteor/client/components/UserInfo/UserInfo.tsx(4 hunks)apps/meteor/client/components/UserInfo/UserInfoABACAttribute.tsx(1 hunks)apps/meteor/client/components/UserInfo/UserInfoABACAttributes.spec.tsx(1 hunks)apps/meteor/client/components/UserInfo/UserInfoABACAttributes.stories.tsx(1 hunks)apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.spec.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.stories.tsx (1)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)
🪛 Biome (2.1.2)
apps/meteor/client/components/UserInfo/UserInfoABACAttribute.tsx
[error] 9-9: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
⏰ 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 (1)
apps/meteor/client/components/UserInfo/UserInfo.tsx (1)
74-75: LGTM! Temporary type annotation is documented.The
ts-expect-errorannotation is clearly documented as temporary until theabacAttributesproperty is added to the User type. The default value ofnullworks well with the conditional rendering below.
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.spec.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx (1)
6-8: The type name still doesn't match the component name.This issue was flagged in a previous review but remains unresolved. The type is named
UserCardABACAttributesPropsbut the component isUserInfoABACAttributes.Apply this diff to align the type name:
-type UserCardABACAttributesProps = { +type UserInfoABACAttributesProps = { abacAttributes: string[]; } & ComponentProps<typeof Box>;And update the usage:
-const UserInfoABACAttributes = ({ abacAttributes }: UserCardABACAttributesProps): ReactElement => { +const UserInfoABACAttributes = ({ abacAttributes }: UserInfoABACAttributesProps): ReactElement => {
🧹 Nitpick comments (4)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx (2)
13-17: Prefer stable keys over array indices.Using array index as key can cause issues if the list is reordered or filtered. Consider using the attribute value itself as the key if attributes are unique.
Apply this diff:
- {abacAttributes.map((attribute, index) => ( - <Box m={2} fontScale='c2' key={index}> + {abacAttributes.map((attribute) => ( + <Box m={2} fontScale='c2' key={attribute}> <UserInfoABACAttribute attribute={attribute} /> </Box> ))}
12-17: Remove redundant styling props. TheUserInfoABACAttributeTag uses itsvariantto apply its ownfontScaleandcolor, so drop thefontScale='p2'andcolor='hint'on the outerBoxand thefontScale='c2'on the innerBoxwrappers—retain only the layout props (e.g.m={2},flexWrap, etc.).apps/meteor/client/components/UserInfo/UserInfo.tsx (2)
74-76: Consider a more idiomatic default value.The prop defaults to
null, which is unusual for an optional array property. Consider usingundefinedor[]instead for consistency with TypeScript conventions.Apply this diff to use undefined (removes the need for the length check):
// @ts-expect-error - abacAttributes is not yet implemented in Users properties - abacAttributes = null, + abacAttributes,Or use an empty array (removes the need for the nullish check):
// @ts-expect-error - abacAttributes is not yet implemented in Users properties - abacAttributes = null, + abacAttributes = [],Then simplify the conditional on Line 181:
- {abacAttributes && abacAttributes.length > 0 && ( + {abacAttributes?.length > 0 && (
183-186: Encapsulate the info icon and tooltip in InfoPanelLabel.Inline Icon and
titlehere stand out (no other usages include inline icons). Add adescriptionprop (or similar) toInfoPanelLabelthat renders the info icon with tooltip, then use:<InfoPanelLabel description={t('ABAC_Attributes_description')}> {t('ABAC_Attributes')} </InfoPanelLabel>Remove the manual
<Icon>andPaletteimport.
📜 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 (2)
apps/meteor/client/components/UserInfo/UserInfo.tsx(4 hunks)apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx(1 hunks)
⏰ 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
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx
Outdated
Show resolved
Hide resolved
MartinSchoeler
force-pushed
the
feat/ABAC-user-attributes
branch
from
8d8d1fc to
d80b906
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx (1)
13-17: Consider using the attribute value as the key instead of index.Using array indices as keys can lead to issues if the list order changes or items are added/removed. Since each attribute string should be unique, consider using the attribute value itself as the key.
Apply this diff:
{abacAttributes.map((attribute, index) => ( - <Margins inline={2} key={index}> + <Margins inline={2} key={attribute}> <UserInfoABACAttribute attribute={attribute} /> </Margins> ))}
📜 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 (1)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx(1 hunks)
⏰ 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 (2)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx (2)
12-12: Address the question about redundant Box styling props.As noted in a previous comment, verify whether the
fontScale='p2'andcolor='hint'props on the Box container are necessary, given thatUserInfoABACAttributeis a Tag component that should have its own proper styling via thevariantprop.If the Tag component already handles text sizing and color through its variant, these container-level props may be redundant or could conflict with the Tag's styling. Please verify the rendered output and remove unnecessary props if they don't affect the layout.
12-12: VerifywithTruncatedTextwith wrapped tags
withTruncatedTextappliesoverflow:hiddenandtext-overflow:ellipsis; combined withflexWrap='wrap', it can hide tags on subsequent lines. Test with multiple ABAC attributes and removewithTruncatedTextif wrapped tags must remain visible.
MartinSchoeler
force-pushed
the
feat/ABAC-user-attributes
branch
from
5acb08c to
cdfcd48
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx (2)
6-8: Box props are declared but never forwarded.The type includes
ComponentProps<typeof Box>but the component doesn't accept or forward these props to the Box element. This creates a misleading API surface.Option 1 (if Box customization is needed): Forward the props:
-const UserInfoABACAttributes = ({ abacAttributes }: UserCardABACAttributesProps): ReactElement => { +const UserInfoABACAttributes = ({ abacAttributes, ...boxProps }: UserCardABACAttributesProps): ReactElement => { return ( - <Box flexWrap='wrap' display='flex'> + <Box flexWrap='wrap' display='flex' {...boxProps}>Option 2 (simpler, if Box props aren't needed): Remove from type:
type UserCardABACAttributesProps = { abacAttributes: string[]; -} & ComponentProps<typeof Box>; +};
6-8: Type name still doesn't match component name.The type remains named
UserCardABACAttributesPropsbut the component isUserInfoABACAttributes. This inconsistency should be addressed.Apply this diff to align the type name:
-type UserCardABACAttributesProps = { +type UserInfoABACAttributesProps = { abacAttributes: string[]; } & ComponentProps<typeof Box>;Then update the component signature:
-const UserInfoABACAttributes = ({ abacAttributes }: UserCardABACAttributesProps): ReactElement => { +const UserInfoABACAttributes = ({ abacAttributes }: UserInfoABACAttributesProps): ReactElement => {
📜 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 (1)
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx(1 hunks)
⏰ 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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
apps/meteor/client/components/UserInfo/UserInfoABACAttributes.tsx
Outdated
Show resolved
Hide resolved
dougfabris
force-pushed
the
feat/ABAC-user-attributes
branch
from
2441885 to
d476132
Compare
dougfabris
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Co-authored-by: Douglas Fabris <27704687+dougfabris@users.noreply.github.com>
Proposed changes (including videos or screenshots)
Adds a new field on the User Info that displays the user's ABAC Attributes
For this PR, only the components are being created, later on we will add the property to the user object and filtering logic (the abacAttributes will not be exactly a
string[])Issue(s)
ABAC-46
Steps to test or reproduce
Further comments
Set the PR as chore since these components are not yet available without mocked data
Summary by CodeRabbit
New Features
Bug Fixes
Tests