-
Notifications
You must be signed in to change notification settings - Fork 2
Feat/env toggle #1192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/env toggle #1192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces an environment toggle feature that allows users to switch between production and current environment for specific features (SAM API calls). The implementation adds localStorage-based feature toggles with visual indicators in the TopBar account section.
Key Changes:
- Added two custom hooks for managing environment toggle state and reading active toggles from localStorage
- Integrated environment avatar and chips into the account UI to visually indicate when non-production features are active
- Created a settings panel in the Account dropdown for toggling features with apply/cancel functionality
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
src/organisms/TopBar/Account/environmentToggles/hooks/useToggleActiveEnvironment.ts |
New hook to read active environment features from localStorage on component mount |
src/organisms/TopBar/Account/environmentToggles/hooks/useEnvironmentToggle.ts |
New hook to manage feature toggle state with save/reset functionality and page reload on apply |
src/organisms/TopBar/Account/EnvironmentAvatar.tsx |
New component displaying a warning-styled avatar when environment features are active |
src/organisms/TopBar/Account/AccountButton.tsx |
Updated to show environment feature chips and prioritize environment avatar over impersonation avatar |
src/organisms/TopBar/Account/AccountAvatar.tsx |
Updated to display environment avatar with label when features are active |
src/organisms/TopBar/Account/Account.tsx |
Added environment toggle switches section with apply/cancel buttons, only shown in non-production |
src/organisms/TopBar/Account/Account.styles.ts |
Added SwitchWrapper styling and updated ButtonWrapper to support multiple buttons in a row |
src/atoms/enums/Environment.ts |
Added PointToProdFeaturesLocalStorageKey enum defining four toggleable features |
Comments suppressed due to low confidence (1)
src/organisms/TopBar/Account/AccountButton.tsx:67
- The avatar display priority might not align with expectations. Currently, environment features take precedence over impersonation (
isActiveFeatureOnCurrentEnvironmentis checked first). This means when both an environment toggle is active AND a user is being impersonated, only the environment avatar shows. Consider if this is the intended behavior, or if impersonation should take priority since it's a more critical security indicator.
{isActiveFeatureOnCurrentEnvironment ? (
<EnvironmentAvatar size={36} />
) : activeImpersonationUser ? (
<ImpersonateAvatar size={36} />
) : (
<ProfileAvatar size={36} name={account?.name} url={photo} />
)}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/organisms/TopBar/Account/environmentToggles/hooks/useEnvironmentToggle.ts
Outdated
Show resolved
Hide resolved
| activeFeaturesOnCurrentEnvironment != null | ||
| ? activeFeaturesOnCurrentEnvironment.length > 0 | ||
| : false, |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check != null is redundant since the state is initialized to [] (not null) and never set to null anywhere in the code. This condition will always be true. Consider simplifying to just check the length: activeFeaturesOnCurrentEnvironment.length > 0.
| activeFeaturesOnCurrentEnvironment != null | |
| ? activeFeaturesOnCurrentEnvironment.length > 0 | |
| : false, | |
| activeFeaturesOnCurrentEnvironment.length > 0, |
src/organisms/TopBar/Account/environmentToggles/hooks/useEnvironmentToggle.ts
Outdated
Show resolved
Hide resolved
src/organisms/TopBar/Account/environmentToggles/hooks/useToggleActiveEnvironment.ts
Outdated
Show resolved
Hide resolved
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/organisms/TopBar/Account/environmentToggles/hooks/useToggleActiveEnvironment.ts
Outdated
Show resolved
Hide resolved
src/organisms/TopBar/Account/environmentToggles/hooks/useToggleActiveEnvironment.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/organisms/TopBar/Account/environmentToggles/hooks/useToggleActiveEnvironment.ts
Outdated
Show resolved
Hide resolved
| {activeFeatures[0].split('-key')[0]} | ||
| </ImpersonationRoleChip> | ||
| )} | ||
| {activeFeatures && activeFeatures.length > 1 && ( |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant null check: The check activeFeatures && is unnecessary since activeFeatures is never null (it's initialized to an empty array).
Simplify to: {activeFeatures.length > 1 && (
| {activeFeatures && activeFeatures.length > 1 && ( | |
| {activeFeatures.length > 1 && ( |
| <EnvironmentToggleWrapper> | ||
| {enableEnvironmentToggle && | ||
| ACTIVE_ENVIRONMENT !== EnvironmentType.PRODUCTION && ( | ||
| <EnvironmentToggle | ||
| setEnvironmentToggle={setEnvironmentToggle} | ||
| environmentToggle={environmentToggle} | ||
| /> | ||
| )} | ||
| </EnvironmentToggleWrapper> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this wrapper be inside the EnvironmentToggle component?
How does it look now if you dont have environment toggle enabled?
src/atoms/utils/environmentToggle.ts
Outdated
| @@ -0,0 +1,3 @@ | |||
| export function getActiveFeatureDisplayName(activeFeature: string) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could take the enum that exists in SAM as a type.
And the util itself could do a replace of "-" to a space " " and maybe capitalize the first letter, and they should look better as names.
And not do the split on "-key", gven that the comments on the SAM PR about changing the names (to "feature-toggle" for example) is done
…tures and improve display name handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const Wrapper = styled.div<StatusVariantProps>` | ||
| width: fit-content; | ||
| border: 2px solid | ||
| ${({ $variant }) => { | ||
| switch ($variant) { | ||
| case 'combined': | ||
| return colors.interactive.success__resting.rgba; | ||
| case 'environment': | ||
| return colors.interactive.success__resting.rgba; | ||
| case 'impersonate': | ||
| return colors.interactive.warning__resting.rgba; | ||
| default: | ||
| return colors.interactive.warning__resting.rgba; | ||
| } | ||
| }}; | ||
| border-radius: 50%; | ||
| > div:first-child { | ||
| background: ${({ $variant }) => { | ||
| switch ($variant) { | ||
| case 'combined': | ||
| return colors.interactive.warning__text.rgba; | ||
| case 'environment': | ||
| return colors.interactive.success__text.rgba; | ||
| case 'impersonate': | ||
| return colors.interactive.warning__text.rgba; | ||
| default: | ||
| return colors.interactive.warning__text.rgba; | ||
| } | ||
| }}; |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is significant code duplication in the switch statement logic for variant-based styling. The same switch statement pattern appears in StatusAvatar.tsx (lines 17-26 and 30-40), AccountButton.tsx (lines 27-38), and AccountAvatar.tsx (lines 28-51). Consider extracting this logic into a shared utility function or constants object to improve maintainability.
| const Wrapper = styled.div<StatusVariantProps>` | |
| width: fit-content; | |
| border: 2px solid | |
| ${({ $variant }) => { | |
| switch ($variant) { | |
| case 'combined': | |
| return colors.interactive.success__resting.rgba; | |
| case 'environment': | |
| return colors.interactive.success__resting.rgba; | |
| case 'impersonate': | |
| return colors.interactive.warning__resting.rgba; | |
| default: | |
| return colors.interactive.warning__resting.rgba; | |
| } | |
| }}; | |
| border-radius: 50%; | |
| > div:first-child { | |
| background: ${({ $variant }) => { | |
| switch ($variant) { | |
| case 'combined': | |
| return colors.interactive.warning__text.rgba; | |
| case 'environment': | |
| return colors.interactive.success__text.rgba; | |
| case 'impersonate': | |
| return colors.interactive.warning__text.rgba; | |
| default: | |
| return colors.interactive.warning__text.rgba; | |
| } | |
| }}; | |
| // Utility functions to map variant to color | |
| function getBorderColorForVariant(variant?: StatusVariant) { | |
| switch (variant) { | |
| case 'combined': | |
| case 'environment': | |
| return colors.interactive.success__resting.rgba; | |
| case 'impersonate': | |
| return colors.interactive.warning__resting.rgba; | |
| default: | |
| return colors.interactive.warning__resting.rgba; | |
| } | |
| } | |
| function getBackgroundColorForVariant(variant?: StatusVariant) { | |
| switch (variant) { | |
| case 'combined': | |
| case 'impersonate': | |
| return colors.interactive.warning__text.rgba; | |
| case 'environment': | |
| return colors.interactive.success__text.rgba; | |
| default: | |
| return colors.interactive.warning__text.rgba; | |
| } | |
| } | |
| const Wrapper = styled.div<StatusVariantProps>` | |
| width: fit-content; | |
| border: 2px solid | |
| ${({ $variant }) => getBorderColorForVariant($variant)}; | |
| border-radius: 50%; | |
| > div:first-child { | |
| background: ${({ $variant }) => getBackgroundColorForVariant($variant)}; |
src/atoms/utils/environmentToggle.ts
Outdated
| export function getActiveFeatureDisplayName( | ||
| activeFeature: EnvironmentToggleFeatures | ||
| ) { | ||
| return ( | ||
| activeFeature.charAt(0).toUpperCase() + | ||
| activeFeature.slice(1).replaceAll('-', ' ') |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name "getActiveFeatureDisplayName" is misleading. The parameter is named "activeFeature", but the function simply formats any string by capitalizing the first letter and replacing hyphens with spaces. It doesn't specifically get or determine which feature is active. Consider renaming to something like "formatFeatureName" or "toDisplayName" to better reflect what it does.
| export function getActiveFeatureDisplayName( | |
| activeFeature: EnvironmentToggleFeatures | |
| ) { | |
| return ( | |
| activeFeature.charAt(0).toUpperCase() + | |
| activeFeature.slice(1).replaceAll('-', ' ') | |
| export function formatFeatureName( | |
| feature: EnvironmentToggleFeatures | |
| ) { | |
| return ( | |
| feature.charAt(0).toUpperCase() + | |
| feature.slice(1).replaceAll('-', ' ') |
| getActiveFeatureDisplayName(item.value as EnvironmentToggleFeatures) | ||
| ); | ||
|
|
||
| const impersonationRoles = activeImpersonationUser?.roles?.sort() ?? []; |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sort() method mutates the original array. This could cause unintended side effects if the roles array is used elsewhere. Consider using toSorted() or creating a copy with slice() before sorting to avoid mutation.
| const impersonationRoles = activeImpersonationUser?.roles?.sort() ?? []; | |
| const impersonationRoles = activeImpersonationUser?.roles?.toSorted() ?? []; |
| return ( | ||
| <StatusAvatar | ||
| size={36} | ||
| name={environmentToggle.map((x) => x.value).join(', ')} |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript may not properly narrow the type of environmentToggle in this block. While isActiveFeatureOnCurrentEnvironment checks that environmentToggle is not null/undefined, TypeScript might still treat it as potentially undefined when calling map() on line 71. Consider using a non-null assertion (environmentToggle!.map) or restructuring the condition to help TypeScript narrow the type.
| name={environmentToggle.map((x) => x.value).join(', ')} | |
| name={environmentToggle!.map((x) => x.value).join(', ')} |
| export const StatusChip = styled(Chip)<StatusVariantProps>` | ||
| background: ${({ $variant }) => { | ||
| switch ($variant) { | ||
| case 'combined': | ||
| return colors.interactive.warning__resting.rgba; | ||
| case 'environment': | ||
| return colors.interactive.success__resting.rgba; | ||
| case 'impersonate': | ||
| return colors.interactive.warning__resting.rgba; | ||
| default: | ||
| return colors.interactive.warning__resting.rgba; | ||
| } | ||
| }}; |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is significant code duplication in the switch statement logic for variant-based styling. The same switch statement pattern appears in StatusAvatar.tsx (lines 17-26 and 30-40), AccountButton.tsx (lines 27-38), and AccountAvatar.tsx (lines 28-51). Consider extracting this logic into a shared utility function or constants object to improve maintainability.
| export const StatusChip = styled(Chip)<StatusVariantProps>` | |
| background: ${({ $variant }) => { | |
| switch ($variant) { | |
| case 'combined': | |
| return colors.interactive.warning__resting.rgba; | |
| case 'environment': | |
| return colors.interactive.success__resting.rgba; | |
| case 'impersonate': | |
| return colors.interactive.warning__resting.rgba; | |
| default: | |
| return colors.interactive.warning__resting.rgba; | |
| } | |
| }}; | |
| // Extracted variant-to-color mapping | |
| const STATUS_VARIANT_COLORS: Record<string, string> = { | |
| combined: colors.interactive.warning__resting.rgba, | |
| environment: colors.interactive.success__resting.rgba, | |
| impersonate: colors.interactive.warning__resting.rgba, | |
| }; | |
| export const StatusChip = styled(Chip)<StatusVariantProps>` | |
| background: ${({ $variant }) => | |
| STATUS_VARIANT_COLORS[$variant ?? 'combined'] || | |
| colors.interactive.warning__resting.rgba}; |
…improve loading state handling
aslakihle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ${({ $variant }) => { | ||
| switch ($variant) { | ||
| case 'combined': | ||
| return colors.interactive.success__resting.rgba; | ||
| case 'environment': | ||
| return colors.interactive.success__resting.rgba; | ||
| case 'impersonate': | ||
| return colors.interactive.warning__resting.rgba; | ||
| default: | ||
| return colors.interactive.warning__resting.rgba; | ||
| } |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The switch statement has identical return values for 'combined', 'impersonate', and 'default' cases. This duplication can be simplified by combining these cases together to improve maintainability.
| background: ${({ $variant }) => { | ||
| switch ($variant) { | ||
| case 'combined': | ||
| return colors.interactive.warning__text.rgba; | ||
| case 'environment': | ||
| return colors.interactive.success__text.rgba; | ||
| case 'impersonate': | ||
| return colors.interactive.warning__text.rgba; | ||
| default: | ||
| return colors.interactive.warning__text.rgba; | ||
| } | ||
| }}; |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The switch statement has identical return values for 'combined', 'impersonate', and 'default' cases. This duplication can be simplified by combining these cases together to improve maintainability.
| setTimeout(() => { | ||
| queryClient.invalidateQueries(); | ||
| setIsLoading(false); | ||
| }, 2000); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded 2000ms timeout value should be extracted as a named constant with a descriptive name (e.g., QUERY_INVALIDATION_DELAY) to improve code maintainability and make the intent clearer.
| setEnvironmentToggle(value); | ||
| setIsLoading(true); | ||
| setTimeout(() => { | ||
| queryClient.invalidateQueries(); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling invalidateQueries without any filter will invalidate all queries in the application, which may cause unnecessary network requests and performance issues. Consider targeting specific query keys that are affected by the environment toggle change.
| queryClient.invalidateQueries(); | |
| queryClient.invalidateQueries(['environment']); |
| background: ${({ $variant }) => { | ||
| switch ($variant) { | ||
| case 'combined': | ||
| return colors.interactive.warning__resting.rgba; | ||
| case 'environment': | ||
| return colors.interactive.success__resting.rgba; | ||
| case 'impersonate': | ||
| return colors.interactive.warning__resting.rgba; | ||
| default: | ||
| return colors.interactive.warning__resting.rgba; | ||
| } | ||
| }}; |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The switch statement has identical return values for 'combined', 'impersonate', and 'default' cases. This duplication can be simplified by combining these cases together to improve maintainability.
| background: ${({ $variant }) => { | ||
| switch ($variant) { | ||
| case 'combined': | ||
| return colors.interactive.warning__resting.rgba; | ||
| case 'environment': | ||
| return colors.interactive.success__resting.rgba; | ||
| case 'impersonate': | ||
| return colors.interactive.warning__resting.rgba; | ||
| default: | ||
| return colors.interactive.warning__resting.rgba; | ||
| } | ||
| }}; | ||
| outline-color: ${({ $variant }) => { | ||
| switch ($variant) { | ||
| case 'combined': | ||
| return colors.interactive.success__resting.rgba; | ||
| case 'environment': | ||
| return colors.interactive.success__resting.rgba; | ||
| case 'impersonate': | ||
| return colors.interactive.warning__resting.rgba; | ||
| default: | ||
| return colors.interactive.warning__resting.rgba; | ||
| } | ||
| }}; |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The switch statement has identical return values for 'combined', 'impersonate', and 'default' cases. This duplication can be simplified by combining these cases together to improve maintainability.
…ariantColors for dynamic styling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…or dynamic styling
aslakihle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
Azure DevOps links
User story
This pull request introduces a new Environment Toggle feature to the Account section, allowing users to select and display active environment-specific features. The implementation includes new UI components, updates to the Account avatar and button to reflect environment and impersonation status, and removes the old
ImpersonateAvatarin favor of a more flexible status-based avatar system. Additionally, dependencies are updated and minor styling improvements are made.EnvironmentTogglecomponent, enabling users to select active environment features from a combobox, with selections persisted in local storage and reflected throughout the Account UI.