feat: Redesign Compare Environments page#7965
Conversation
- Convert CompareEnvironments from JS to TypeScript - Add expandable rows with full diff view (segments, variations) - Add standard table filters (search, tags, owners, groups, sort) - Split results into "Changed features" and "Unchanged features" tables - Add column headers for source/target environments and segment changes - Show environment names in diff view dropdown instead of "Old/New Value" - Add environment name legend with +/- indicators in diff colours - Style improvements: proper table header styling, badge counts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe legacy CompareEnvironments component is replaced by a new TypeScript comparison page split into a page component, a row component, an expanded diff component, a comparison hook, and shared types. The new flow fetches and compares feature data for two environments, filters and renders changed and unchanged items, and supports row expansion and feature editing. DiffFeature now accepts optional environment names, and matching header styles were added. Estimated code review effort: 5 (Critical) | ~90 minutes Comment |
github-actions
Bot
added
the
front-end
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d35e7802-5b0d-4d36-a807-ab7858d6883c
📒 Files selected for processing (4)
frontend/web/components/CompareEnvironments.jsfrontend/web/components/CompareEnvironments.tsxfrontend/web/components/diff/DiffFeature.tsxfrontend/web/styles/3rdParty/_react-diff.scss
💤 Files with no reviewable changes (1)
- frontend/web/components/CompareEnvironments.js
| @@ -0,0 +1,532 @@ | |||
| import React, { FC, useCallback, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
New component should live in its own folder with a barrel index.ts.
This new component is placed directly under components/. Per the component-structure guideline it should be CompareEnvironments/CompareEnvironments.tsx with a co-located barrel index.ts (and any subcomponents/SCSS co-located), with imports referencing the folder path.
As per coding guidelines: "Each new component must live in its own folder with a barrel index.ts, a ComponentName/ComponentName.tsx file, co-located ComponentName.scss, any subcomponents, and imports should use the component folder path rather than the inner file."
Source: Coding guidelines
There was a problem hiding this comment.
I agree with this comment. We already have a few components (ExpandedRow is defined in-file, renderRow is a component in disguise). Could it move to its own folder with a barrel and split by concern in order to improve maintenance ?
web/components/compare-environments/
index.ts // re-exports default
CompareEnvironments.tsx // state + layout
CompareFeatureRow.tsx // the renderRow content
ExpandedRow.tsx
useEnvironmentComparison.ts // fetch + diff derivation
types.ts // FeatureChange
Pulling the fetch/diff logic into a useEnvironmentComparison hook would also make it unit-testable.
There was a problem hiding this comment.
@talissoncosta That structure looks solid and aligns well with the component guideline (folder + barrel index.ts, plus co-located CompareEnvironments.scss if any styles get extracted too). A few small notes on the breakdown:
useEnvironmentComparison.tspulling out fetch + diff derivation is a good call — it isolates the RTK Query calls (useGetFeatureStatesQuery, project feature metadata fetches) and the diff/filter/sort logic from rendering, making it independently testable with mocked query results.CompareFeatureRow.tsxfor therenderRowlogic andExpandedRow.tsx(already identified in this PR's diff around theExpandedRowcomponent) both make sense as their own files since they have distinct responsibilities (row summary vs. expanded diff view).types.tsforFeatureChange(and theExpandedRowPropstype) keeps shared shapes reusable across the split files without circular imports.- Since
CompareEnvironmentsis used elsewhere (referenced by route/other components), double-check those import paths get updated toweb/components/compare-environmentsonce the barrel is in place.
If you'd like, I can draft this refactor (folder/file split + barrel export) as a follow-up — just let me know and I'll put together the plan.
| leftValue: string | number | boolean | null | ||
| leftEnvironmentFlag: FeatureState | ||
| rightEnabled: boolean | ||
| rightValue: string | number | boolean | null |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the inline union into a named type.
leftValue/rightValue use an inline string | number | boolean | null union. This matches the existing FlagsmithValue type used for feature_state_value; reuse it (or introduce a named alias).
♻️ Proposed change
- leftValue: string | number | boolean | null
+ leftValue: FlagsmithValue
...
- rightValue: string | number | boolean | null
+ rightValue: FlagsmithValue(import FlagsmithValue from common/types/responses)
As per coding guidelines: "Extract inline union types into named types."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| leftValue: string | number | boolean | null | |
| leftEnvironmentFlag: FeatureState | |
| rightEnabled: boolean | |
| rightValue: string | number | boolean | null | |
| leftValue: FlagsmithValue | |
| leftEnvironmentFlag: FeatureState | |
| rightEnabled: boolean | |
| rightValue: FlagsmithValue |
Source: Coding guidelines
Visual Regression19 screenshots compared. See report for details. |
Add Edit buttons to each environment column that open the CreateFlagModal for editing features directly from the comparison view. The modal handles change requests automatically when required. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-actions
Bot
added
feature
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e5c607a-0133-4beb-977e-e3136eda0457
📒 Files selected for processing (1)
frontend/web/components/CompareEnvironments.tsx
talissoncosta
left a comment
talissoncosta
left a comment
There was a problem hiding this comment.
Looks good @kyle-ssg ! Thanks for that!
I just left some suggestions.
If we can break it into small components and get rid of the _data import would be amazing.
| @@ -0,0 +1,532 @@ | |||
| import React, { FC, useCallback, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
I agree with this comment. We already have a few components (ExpandedRow is defined in-file, renderRow is a component in disguise). Could it move to its own folder with a barrel and split by concern in order to improve maintenance ?
web/components/compare-environments/
index.ts // re-exports default
CompareEnvironments.tsx // state + layout
CompareFeatureRow.tsx // the renderRow content
ExpandedRow.tsx
useEnvironmentComparison.ts // fetch + diff derivation
types.ts // FeatureChange
Pulling the fetch/diff logic into a useEnvironmentComparison hook would also make it unit-testable.
Address PR review feedback on the Compare Environments redesign: - Move the single 607-line component into a CompareEnvironments/ folder with a barrel: types.ts, useEnvironmentComparison hook (fetch + diff derivation, now unit-testable), ExpandedRow, CompareFeatureRow, and the layout component. - Replace relative imports with components/ and common/ aliases. - Reuse the shared FlagsmithValue type instead of an inline union. - Surface a fetch error state with a retry action instead of silently swallowing failures and rendering "No differences found". - Guard against stale overlapping comparisons with a request-id check so a slow earlier response can't overwrite a newer one. - Make the row expand toggle keyboard accessible (role, tabIndex, aria-expanded, Enter/Space handling). - Pass the numeric projectId into CreateFlagModal. - Fall back to the left project flag for the right-side edit action so the modal edits the existing feature instead of switching to create mode. - Consolidate duplicated diff header styles into a shared base rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
feature
There was a problem hiding this comment.
Actionable comments posted: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9358fdf4-3fea-4572-8fe1-3a268386a44c
📒 Files selected for processing (7)
frontend/web/components/CompareEnvironments/CompareEnvironments.tsxfrontend/web/components/CompareEnvironments/CompareFeatureRow.tsxfrontend/web/components/CompareEnvironments/ExpandedRow.tsxfrontend/web/components/CompareEnvironments/index.tsfrontend/web/components/CompareEnvironments/types.tsfrontend/web/components/CompareEnvironments/useEnvironmentComparison.tsfrontend/web/styles/3rdParty/_react-diff.scss
| import ExpandedRow from './ExpandedRow' | ||
| import { FeatureChange } from './types' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Relative imports violate import-path guideline.
As per coding guidelines, "Use only common/, components/, and project/ import paths; do not use relative imports."
♻️ Proposed fix
-import ExpandedRow from './ExpandedRow'
-import { FeatureChange } from './types'
+import ExpandedRow from 'components/CompareEnvironments/ExpandedRow'
+import { FeatureChange } from 'components/CompareEnvironments/types'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ExpandedRow from './ExpandedRow' | |
| import { FeatureChange } from './types' | |
| import ExpandedRow from 'components/CompareEnvironments/ExpandedRow' | |
| import { FeatureChange } from 'components/CompareEnvironments/types' |
Source: Coding guidelines
| onKeyDown={(e: React.KeyboardEvent) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault() | ||
| toggle() | ||
| } | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keyboard "Enter" on Edit buttons also toggles the row.
The row's onKeyDown handles Enter/Space to toggle expansion. The Edit buttons only call stopPropagation() in onClick, not on the native keydown — which bubbles to the parent before the synthesized click fires. Using the keyboard to activate "Edit" therefore both opens the edit flow and expands/collapses the row.
🐛 Proposed fix
<div
className='table-column d-flex align-items-center gap-2'
style={{ width: 280 }}
+ onKeyDown={(e: React.KeyboardEvent) => e.stopPropagation()}
>
<Switch checked={item.leftEnabled} disabled />
...
<div
className='table-column d-flex align-items-center gap-2'
style={{ width: 280 }}
+ onKeyDown={(e: React.KeyboardEvent) => e.stopPropagation()}
>
<Switch checked={item.rightEnabled} disabled />Also applies to: 87-101, 111-127
| import { useGetFeatureStatesQuery } from 'common/services/useFeatureState' | ||
| import { FeatureStateWithConflict } from 'common/types/responses' | ||
| import DiffFeature from 'components/diff/DiffFeature' | ||
| import { FeatureChange } from './types' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Relative import violates import-path guideline.
As per coding guidelines, "Use only common/, components/, and project/ import paths; do not use relative imports."
♻️ Proposed fix
-import { FeatureChange } from './types'
+import { FeatureChange } from 'components/CompareEnvironments/types'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { FeatureChange } from './types' | |
| import { FeatureChange } from 'components/CompareEnvironments/types' |
Source: Coding guidelines
| const { data: leftStates, isLoading: leftLoading } = useGetFeatureStatesQuery( | ||
| { | ||
| environment: environmentLeftId, | ||
| feature: item.projectFlagLeft.id, | ||
| }, | ||
| ) | ||
|
|
||
| const { data: rightStates, isLoading: rightLoading } = | ||
| useGetFeatureStatesQuery({ | ||
| environment: environmentRightId, | ||
| feature: item.projectFlagLeft.id, | ||
| }) | ||
|
|
||
| if (leftLoading || rightLoading) { | ||
| return ( | ||
| <div className='p-4 text-center'> | ||
| <Loader /> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fetch failures are silently swallowed.
isError isn't read from either query; on failure the row falls straight to DiffFeature with empty arrays and shows "No differences between environments", hiding the actual failure from the user.
🛡️ Proposed fix
- const { data: leftStates, isLoading: leftLoading } = useGetFeatureStatesQuery(
+ const {
+ data: leftStates,
+ isLoading: leftLoading,
+ isError: leftError,
+ } = useGetFeatureStatesQuery(
{
environment: environmentLeftId,
feature: item.projectFlagLeft.id,
},
)
- const { data: rightStates, isLoading: rightLoading } =
+ const { data: rightStates, isLoading: rightLoading, isError: rightError } =
useGetFeatureStatesQuery({
environment: environmentRightId,
feature: item.projectFlagLeft.id,
})
if (leftLoading || rightLoading) {
return (
<div className='p-4 text-center'>
<Loader />
</div>
)
}
+
+ if (leftError || rightError) {
+ return (
+ <div className='p-4 text-center text-danger'>
+ Failed to load feature state comparison.
+ </div>
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { data: leftStates, isLoading: leftLoading } = useGetFeatureStatesQuery( | |
| { | |
| environment: environmentLeftId, | |
| feature: item.projectFlagLeft.id, | |
| }, | |
| ) | |
| const { data: rightStates, isLoading: rightLoading } = | |
| useGetFeatureStatesQuery({ | |
| environment: environmentRightId, | |
| feature: item.projectFlagLeft.id, | |
| }) | |
| if (leftLoading || rightLoading) { | |
| return ( | |
| <div className='p-4 text-center'> | |
| <Loader /> | |
| </div> | |
| ) | |
| } | |
| const { | |
| data: leftStates, | |
| isLoading: leftLoading, | |
| isError: leftError, | |
| } = useGetFeatureStatesQuery( | |
| { | |
| environment: environmentLeftId, | |
| feature: item.projectFlagLeft.id, | |
| }, | |
| ) | |
| const { data: rightStates, isLoading: rightLoading, isError: rightError } = | |
| useGetFeatureStatesQuery({ | |
| environment: environmentRightId, | |
| feature: item.projectFlagLeft.id, | |
| }) | |
| if (leftLoading || rightLoading) { | |
| return ( | |
| <div className='p-4 text-center'> | |
| <Loader /> | |
| </div> | |
| ) | |
| } | |
| if (leftError || rightError) { | |
| return ( | |
| <div className='p-4 text-center text-danger'> | |
| Failed to load feature state comparison. | |
| </div> | |
| ) | |
| } |
| import ProjectStore from 'common/stores/project-store' | ||
| import { hasMultivariateChange } from 'common/utils/compareMultivariate' | ||
| import { Environment, FeatureState, ProjectFlag } from 'common/types/responses' | ||
| import { FeatureChange } from './types' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Relative import violates import-path guideline.
Use an absolute component-folder path instead of a relative import.
As per coding guidelines, "Use only common/, components/, and project/ import paths; do not use relative imports."
♻️ Proposed fix
-import { FeatureChange } from './types'
+import { FeatureChange } from 'components/CompareEnvironments/types'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { FeatureChange } from './types' | |
| import { FeatureChange } from 'components/CompareEnvironments/types' |
Source: Coding guidelines
| try { | ||
| const [leftProjectFlags, rightProjectFlags, leftFlags, rightFlags] = | ||
| await Promise.all([ | ||
| data.get( | ||
| `${Project.api}projects/${projectId}/features/?page_size=999&environment=${leftEnvironmentId}`, | ||
| ), | ||
| data.get( | ||
| `${Project.api}projects/${projectId}/features/?page_size=999&environment=${rightEnvironmentId}`, | ||
| ), | ||
| data.get( | ||
| `${Project.api}environments/${leftEnvironmentKey}/featurestates/?page_size=999`, | ||
| ), | ||
| data.get( | ||
| `${Project.api}environments/${rightEnvironmentKey}/featurestates/?page_size=999`, | ||
| ), | ||
| ]) | ||
|
|
||
| if (currentRequest !== requestId.current) return | ||
|
|
||
| setChanges( | ||
| deriveChanges( | ||
| leftProjectFlags.results || [], | ||
| rightProjectFlags.results || [], | ||
| leftFlags.results || [], | ||
| rightFlags.results || [], | ||
| ), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Direct data.get() calls bypass the RTK Query requirement.
The hook fetches project flags and feature states via data.get() on raw URLs rather than injecting RTK Query endpoints. ExpandedRow.tsx already uses useGetFeatureStatesQuery for the same feature-state resource, so this hook diverges from the established pattern and loses caching, deduplication, typed responses, and consistent error handling that RTK Query provides.
As per coding guidelines, "Never use fetch() directly; use RTK Query mutations/queries instead, injecting endpoints into services in common/services/."
Flagsmith frontend RTK Query useFeatureState endpoints project flags list
Source: Coding guidelines
| const leftEnvironment = ProjectStore.getEnvironment(leftEnvironmentKey) as | ||
| | Environment | ||
| | undefined | ||
| | null | ||
| const rightEnvironment = ProjectStore.getEnvironment(rightEnvironmentKey) as | ||
| | Environment | ||
| | undefined | ||
| | null |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the duplicated inline union type.
Environment | undefined | null is written out twice; extract it to a named type.
As per coding guidelines, "Extract inline union types into named types."
♻️ Proposed fix
+type ProjectEnvironment = Environment | undefined | null
+
export const useEnvironmentComparison = ({
...
}: UseEnvironmentComparisonArgs) => {
...
- const leftEnvironment = ProjectStore.getEnvironment(leftEnvironmentKey) as
- | Environment
- | undefined
- | null
- const rightEnvironment = ProjectStore.getEnvironment(rightEnvironmentKey) as
- | Environment
- | undefined
- | null
+ const leftEnvironment = ProjectStore.getEnvironment(leftEnvironmentKey) as ProjectEnvironment
+ const rightEnvironment = ProjectStore.getEnvironment(rightEnvironmentKey) as ProjectEnvironment📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const leftEnvironment = ProjectStore.getEnvironment(leftEnvironmentKey) as | |
| | Environment | |
| | undefined | |
| | null | |
| const rightEnvironment = ProjectStore.getEnvironment(rightEnvironmentKey) as | |
| | Environment | |
| | undefined | |
| | null | |
| type ProjectEnvironment = Environment | undefined | null | |
| export const useEnvironmentComparison = ({ | |
| ... | |
| }: UseEnvironmentComparisonArgs) => { | |
| ... | |
| const leftEnvironment = ProjectStore.getEnvironment(leftEnvironmentKey) as ProjectEnvironment | |
| const rightEnvironment = ProjectStore.getEnvironment(rightEnvironmentKey) as ProjectEnvironment |
Source: Coding guidelines
…PageGet Export the existing recursivePageGet helper and reuse it in the comparison hook so every list is fetched across all pages, replacing the page_size=999 calls that silently capped results at 999. A small _data.get adapter bridges it to the baseQuery contract the helper expects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/common/services/useProjectFlag.ts (1)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
recursivePageGetgeneric
It’s reused for bothProjectFlagandFeatureState, so parameterising the response type will remove the cast inuseEnvironmentComparisonand keep callers type-safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7b13097-571d-4572-a88c-8cca942142a7
📒 Files selected for processing (2)
frontend/common/services/useProjectFlag.tsfrontend/web/components/CompareEnvironments/useEnvironmentComparison.ts
github-actions
Bot
added
feature
talissoncosta
left a comment
talissoncosta
left a comment
There was a problem hiding this comment.
Thanks for addressing the changes.
It looks good!
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Please describe.
How did you test this code?
Compared features in production