Update taxonomy UI for custom field improvements #7043
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile OverviewGreptile SummaryThis PR implements custom taxonomy UI improvements that allow custom fields to be applied to custom taxonomies and enable using taxonomy keys as field types. The primary changes involve migrating from strict enum types ( Important Files Changed
Confidence score: 4/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { isIdentityProviderColumns } from "../utils/columnBuilders"; | ||
|
|
||
| interface UseDiscoveredInfrastructureSystemsColumnsProps { | ||
| isOktaApp?: boolean; |
There was a problem hiding this comment.
logic: The isOktaApp prop is defined in the interface but not used in the hook implementation. Is the isOktaApp prop intended to be passed to the isIdentityProviderColumns function or used elsewhere?
| if (customFields.isEnabled && resourceType) { | ||
| if (customFields.isEnabled) { | ||
| const customFieldValues = customFieldsForm.getFieldsValue(); | ||
| console.log("customFieldValues", customFieldValues); |
There was a problem hiding this comment.
style: Debug console.log should be removed before merging to production
| import type { Constraint } from "./Constraint"; | ||
| import type { DiffStatus } from "./DiffStatus"; | ||
| import { IdentityProviderApplicationMetadata } from "./IdentityProviderApplicationMetadata"; | ||
| import type { ResourceError } from "./ResourceError"; |
There was a problem hiding this comment.
style: The ResourceError import is unused in this file and should be removed to keep imports clean.
| import type { ResourceError } from "./ResourceError"; | |
| import { IdentityProviderApplicationMetadata } from "./IdentityProviderApplicationMetadata"; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| label: group.name, | ||
| })); | ||
|
|
||
| // eslint-disable-next-line jsx-a11y/control-has-associated-label |
There was a problem hiding this comment.
style: This eslint disable comment appears unnecessary since the Select component from Ant Design should properly handle accessibility labels through its internal implementation and the placeholder prop provides accessible context.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (isCustomTaxonomiesLoading) { | ||
| return { valueTypeOptions, customTaxonomies: [] }; | ||
| } |
There was a problem hiding this comment.
logic: return type is inconsistent - returns customTaxonomies when loading but not in the final return
| if (!taxonomyType || !fidesKey) { | ||
| messageApi.error("Taxonomy type not found"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
style: Error handling could be more specific - 'Taxonomy type not found' message appears when either taxonomyType or fidesKey is missing, but the message doesn't distinguish between these cases
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "app_type": "SAML_2_0", | ||
| "status": "ACTIVE", | ||
| "created": "2023-06-15T09:00:00Z", | ||
| "sign_on_url": "https://company.okta.com/app/salesforce/sso/saml", |
There was a problem hiding this comment.
style: Uses real domain (company.okta.com) instead of reserved test domain
| "sign_on_url": "https://company.okta.com/app/salesforce/sso/saml", | |
| "sign_on_url": "https://company.example.com/app/salesforce/sso/saml", |
Context Used: Context from dashboard - .cursor/rules/sensitive-info-protection.mdc (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "created": "2023-06-15T09:00:00Z", | ||
| "sign_on_url": "https://company.okta.com/app/salesforce/sso/saml", | ||
| "vendor_match_confidence": "medium", | ||
| "vendor_logo_url": "https://logo.clearbit.com/salesforce.com" |
There was a problem hiding this comment.
style: Uses real domain (salesforce.com) for logo URL instead of reserved test domain
| "vendor_logo_url": "https://logo.clearbit.com/salesforce.com" | |
| "vendor_logo_url": "https://logo.clearbit.com/example.com" |
Context Used: Context from dashboard - .cursor/rules/sensitive-info-protection.mdc (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const { data, isLoading, isFetching } = useGetAllUsersQuery({ | ||
| page: pageIndex, | ||
| size: pageSize, | ||
| username: searchQuery || undefined, |
There was a problem hiding this comment.
style: Consider using nullish coalescing operator (??) instead of logical OR since empty string is a valid search query
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const permissionsLabels: string[] = []; | ||
| if (userPermissions && userPermissions.roles) { | ||
| userPermissions.roles.forEach((permissionRole) => { | ||
| const matchingRole = ROLES.find( | ||
| (role) => role.roleKey === permissionRole, | ||
| ); | ||
| if (matchingRole) { | ||
| permissionsLabels.push(matchingRole.permissions_label); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
style: Consider using filter and map chain instead of imperative forEach approach for better functional programming style
Context Used: Rule from dashboard - Prefer simpler code implementations when they achieve the same functionality, such as using `filter(... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@jpople something seems off with this PR. It appears to include work from other recent merges? Probably a bad rebase or something. |
Ticket ENG-1262
See #6898 for description.
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works