remove data manipulation of table lists from frontend#3104
Conversation
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
This PR shifts list/table manipulation responsibilities (sorting/filtering/pagination state) from the Angular frontend to the backend and NgRx store, replacing MatTableDataSource-based client-side behavior with plain array data sources while preserving backend ordering.
Changes:
- Refactors Segment / Feature Flag / Experiment “root section card” tables to consume
Observable<T[]>/T[]directly instead ofMatTableDataSource<T>, and routes sorting actions to backend fetches. - Reworks NgRx state for experiments/feature flags/segments away from
@ngrx/entityto plain arrays to preserve backend sort order. - Moves “exclude archived experiments unless searching by status” logic to the backend and adjusts request validation to allow blank search strings.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card.component.ts | Switches segment list binding to direct segments observable and removes table helper/data source wiring. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card.component.html | Passes segments stream to the table component. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card-table/segment-root-section-card-table.component.ts | Removes client-side sort/data-source logic; triggers backend fetch on sort and scroll. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card-table/segment-root-section-card-table.component.html | Uses async array datasource instead of MatTableDataSource. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-root-page/feature-flag-root-page-content/feature-flag-root-section-card/feature-flag-root-section-card.component.ts | Replaces MatTableDataSource plumbing with an observable list from the service. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-root-page/feature-flag-root-page-content/feature-flag-root-section-card/feature-flag-root-section-card.component.html | Passes feature flag stream to the table component. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-root-page/feature-flag-root-page-content/feature-flag-root-section-card/feature-flag-root-section-card-table/feature-flag-root-section-card-table.component.ts | Removes client-side sorting; fetches from backend on sort changes; uses array datasource. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-root-page/feature-flag-root-page-content/feature-flag-root-section-card/feature-flag-root-section-card-table/feature-flag-root-section-card-table.component.html | Uses async array datasource instead of MatTableDataSource. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-root-page/experiment-root-page-content/experiment-root-section-card/experiment-root-section-card.component.ts | Switches to direct experiments observable; removes frontend filtering of archived experiments. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-root-page/experiment-root-page-content/experiment-root-section-card/experiment-root-section-card.component.html | Passes resolved experiments array to the table component. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-root-page/experiment-root-page-content/experiment-root-section-card/experiment-root-section-card-table/experiment-root-section-card-table.component.ts | Removes MatTableDataSource usage; routes sorting to backend fetches. |
| packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-root-page/experiment-root-page-content/experiment-root-section-card/experiment-root-section-card-table/experiment-root-section-card-table.component.html | Uses array datasource instead of MatTableDataSource. |
| packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.ts | Updates selectors to read from array-based segment state. |
| packages/frontend/projects/upgrade/src/app/core/segments/store/segments.selectors.spec.ts | Updates selector tests for array-based state. |
| packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.ts | Replaces entity adapter state with array-based storage/pagination updates. |
| packages/frontend/projects/upgrade/src/app/core/segments/store/segments.reducer.spec.ts | Updates reducer tests for array-based state. |
| packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts | Updates SegmentState/GlobalSegmentState to array-based shapes. |
| packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts | Removes client-side sorting in allSegments$ to preserve backend order. |
| packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.ts | Updates initial persisted state shape for experiments/feature flags/segments. |
| packages/frontend/projects/upgrade/src/app/core/local-storage/local-storage.service.spec.ts | Updates local-storage unit tests for new experiments state shape. |
| packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts | Updates selectors for array-based list state + selected-flag handling. |
| packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts | Replaces entity adapter usage with array-based list + selected-flag state. |
| packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts | Updates FeatureFlagState to separate list vs selected flag. |
| packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts | Adds featureFlags$ selector-backed stream for list rendering. |
| packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts | Updates experiment selectors to use array-based state. |
| packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selector.spec.ts | Updates selector tests for array-based state. |
| packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts | Replaces entity adapter state with array-based list state and updates. |
| packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.spec.ts | Updates reducer tests for array-based state behavior. |
| packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts | Updates ExperimentState to store experiments as an array. |
| packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts | Always sends searchParams (including blank string) for experiment pagination requests. |
| packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts | Removes frontend sorting to preserve backend ordering. |
| packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.spec.ts | Updates service test expectation to reflect selector order. |
| packages/backend/src/api/services/ExperimentService.ts | Applies archived-experiment filtering on the backend and avoids applying search when string is blank. |
| packages/backend/src/api/controllers/validators/ExperimentPaginatedParamsValidator.ts | Allows empty search strings by removing @IsNotEmpty() on search string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
|
double checking, is it right for it show archived when status is first selected (or when no status typed yet)? It looks like it does that in QA also, so may not be related to this change. 
|
| on(SegmentsActions.actionGetSegmentByIdSuccess, (state, { segment }) => { | ||
| if (segment.type === SEGMENT_TYPE.GLOBAL_EXCLUDE) { | ||
| return adapter.upsertOne(segment, { ...state, isLoadingSegments: false }); | ||
| return { ...state, isLoadingSegments: false }; |
There was a problem hiding this comment.
both return paths are the same now actually, looks like it can just be return { ...state, isLoadingSegments: false }
| exclusion.segment.id === listResponse.segment.id ? listResponse : exclusion | ||
| ) ?? []; | ||
| // Update selectedFlag if it matches the modified flag | ||
| const updatedSelectedFlag = |
There was a problem hiding this comment.
&& state.selectedFlag not needed
There was a problem hiding this comment.
Woudn't it evaluate to true if they're both undefined?
| ) ?? []; | ||
| // Update selectedFlag if it matches the modified flag | ||
| const updatedSelectedFlag = | ||
| state.selectedFlag?.id === featureFlag?.id && state.selectedFlag |
There was a problem hiding this comment.
can this and the experiment equivalent be refactored a little to avoid a chain of operators? claude suggests something like:
if (state.selectedFlag?.id !== featureFlag?.id) {
return { ...state, isLoadingUpsertPrivateSegmentList: false };
}
const updatedInclusions = state.selectedFlag.featureFlagSegmentInclusion.map((item) =>
item.segment.id === listResponse.segment.id ? listResponse : item
);
return {
...state,
selectedFlag: { ...state.selectedFlag, featureFlagSegmentInclusion: updatedInclusions },
isLoadingUpsertPrivateSegmentList: false,
};There was a problem hiding this comment.
The above suggestion doesn't do quite the same thing; for instance, it will try and fail to dereference state.selectedFlag.featureFlagSegmentInclusion if both state.selectedFlag?.id and featureFlag?.id evaluate to null or undefined. There are many places where this reducer idiom is used. Would we want them all to be changed? Maybe a refactor ticket?
| @@ -61,7 +61,7 @@ export class ExperimentRootSectionCardTableComponent implements OnInit { | |||
| constructor(private readonly experimentService: ExperimentService) {} | |||
|
|
|||
| ngOnInit() { | |||
There was a problem hiding this comment.
we can just remove these lifecycle hooks if we're not using them, they don't have to be there
| @@ -59,15 +59,15 @@ export class FeatureFlagRootSectionCardTableComponent implements OnInit { | |||
| constructor(private featureFlagsService: FeatureFlagsService) {} | |||
|
|
|||
| ngOnInit() { | |||
| // When sorting is cleared, revert to default sorting | ||
| this.featureFlagsService.setSortingType(null); | ||
| this.featureFlagsService.setSortKey(null); | ||
| this.featureFlagsService.fetchFeatureFlags(true); // true = reset pagination |
There was a problem hiding this comment.
what if we updated this fetch pattern globally to explain the mysterious "true" param of these fetches from this.featureFlagsService.fetchFeatureFlags(true) to this.featureFlagsService.fetchFeatureFlags({ fromStart: true }) so its obvious and we don't need the comment? doesn't have to be in this pr, i can put something up if you agree.
There was a problem hiding this comment.
Makes sense to me. Maybe not for this pr
| <div class="table-container"> | ||
| <app-feature-flag-root-section-card-table | ||
| [dataSource$]="dataSource$ | async" | ||
| [featureFlags$]="featureFlags$" |
There was a problem hiding this comment.
it's not async anymore? or was this pipe always not needed?
There was a problem hiding this comment.
The data doesn't need to be unwrapped in the card component anymore, so the observable is just passed through and the 'async' was moved to the table component.
|
this is fascinating. i wouldn't have thought to remove the 'adapter' things, i never really thought about why we needed it but yeah i guess if this works and we always refetch... i guess they didn't really do anything useful... my comments are mostly nitty things, i think everything looks to be working but I'd like to play around a little more tomorrow to see if i can break it :) |
Yeah, that's the current (desired) behavior. |
Yeah, It's a separation of concerns thing. The adapters, and the 'entities' they worked with, were there to make sure that edits to the experiments, etc. were reflected in the lists being maintained in the state. I found that the adapter/entity pattern can control the order of lists in ways that are not so obvious. But the lists should always reflect what we're getting from the backend. Since edits/adds/deletes are always performed on the details page, and we refetch when returning to the lists, it made sense to rely only on the backend for the state of the lists, but continue using the UI-based state updates only for the details pages. |
Co-authored-by: Copilot <copilot@github.com>
No description provided.