refactor: update frontend to be compatible with the new externalsettings structure#2274
Open
refactor: update frontend to be compatible with the new externalsettings structure#2274
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- There are multiple places where you branch on
conditionSettingsKeyinSharedConditionComponent(e.g. inngOnInit,updateStore,updateConditionField); consider centralizing this mapping (e.g. a lookup object from key → selector/dispatchers) to reduce duplication and the risk of inconsistencies when new condition types are added. - The list of external settings keys (e.g. in
settingsToCheckinsideUserEffects) is currently hard-coded; extracting this into a shared typed constant or enum would make it easier to keep frontend logic aligned with the backendexternalSettingsstructure as it evolves.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are multiple places where you branch on `conditionSettingsKey` in `SharedConditionComponent` (e.g. in `ngOnInit`, `updateStore`, `updateConditionField`); consider centralizing this mapping (e.g. a lookup object from key → selector/dispatchers) to reduce duplication and the risk of inconsistencies when new condition types are added.
- The list of external settings keys (e.g. in `settingsToCheck` inside `UserEffects`) is currently hard-coded; extracting this into a shared typed constant or enum would make it easier to keep frontend logic aligned with the backend `externalSettings` structure as it evolves.
## Individual Comments
### Comment 1
<location path="src/app/state-management/effects/user.effects.ts" line_range="405-414" />
<code_context>
+ setConditions$ = createEffect(() => {
</code_context>
<issue_to_address>
**question (bug_risk):** Re-evaluate removing selectColumnAction when restoring conditions from settings
Previously this effect also dispatched `selectColumnAction` for each enabled condition so the related columns became visible when restoring from settings. Now it only dispatches `addScientificConditionAction` and `updateConditionsConfigs`, which may drop that auto-enable behavior. Please confirm whether the UI still depends on this, and if the change is intentional, ensure there’s a clear mechanism to keep columns and conditions in sync.
</issue_to_address>
### Comment 2
<location path="src/app/state-management/effects/user.effects.ts" line_range="332" />
<code_context>
+ ...(userSettings.externalSettings || {}),
+ };
+
+ const settingsToCheck = [
+ "fe_dataset_table_columns",
+ "fe_dataset_table_conditions",
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing shared helper utilities and a centralised settings configuration map so the new external settings logic is declarative, typed, and reused across effects instead of relying on repeated string operations and inline branching.
You can reduce the new complexity by centralising the stringly-typed logic and the “which conditions to use” decision into small, reusable helpers.
### 1. Replace `startsWith`/`replace` chains with a typed settings config
Define a single source of truth for external setting keys and their scopes:
```ts
const SETTINGS_CONFIG = [
{ key: "fe_dataset_table_columns", scope: "dataset", configKey: "columns" },
{ key: "fe_dataset_table_conditions", scope: "dataset", configKey: "conditions" },
{ key: "fe_dataset_table_filters", scope: "dataset", configKey: "filters" },
{ key: "fe_proposal_table_columns", scope: "proposal", configKey: "columns" },
{ key: "fe_proposal_table_filters", scope: "proposal", configKey: "filters" },
{ key: "fe_sample_table_columns", scope: "sample", configKey: "columns" },
{ key: "fe_sample_table_conditions", scope: "sample", configKey: "conditions" },
{ key: "fe_instrument_table_columns", scope: "instrument", configKey: "columns" },
{ key: "fe_file_table_columns", scope: "file", configKey: "columns" },
] as const;
type SettingsKey = (typeof SETTINGS_CONFIG)[number]["key"];
```
Then extract a normalisation helper used by `fetchUserSettings$`:
```ts
function normalizeExternalSetting(
settingKey: SettingsKey,
externalSettings: Record<string, any>,
config: any,
initialUserState: typeof initialUserState,
) {
let items = externalSettings[settingKey];
if (Array.isArray(items) && items.length > 0) {
return items;
}
const cfg = SETTINGS_CONFIG.find((s) => s.key === settingKey);
if (!cfg) {
return initialUserState.settings[settingKey] || [];
}
const defaultsByScope: Record<string, any> = {
dataset: config.defaultDatasetsListSettings,
proposal: config.defaultProposalsListSettings,
// sample / instrument / file could use their own config section if available
};
const scopeDefaults = defaultsByScope[cfg.scope] ?? {};
return (
scopeDefaults?.[cfg.configKey] ||
initialUserState.settings[settingKey] ||
[]
);
}
```
Usage in `fetchUserSettings$` (no `startsWith`/`replace`):
```ts
map((userSettings: UserSettings) => {
const config = this.configService.getConfig();
const externalSettings = { ...(userSettings.externalSettings || {}) };
for (const { key } of SETTINGS_CONFIG) {
externalSettings[key] = normalizeExternalSetting(
key,
externalSettings,
config,
initialUserState,
);
}
const normalizedUserSettings = {
...userSettings,
externalSettings,
};
return fromActions.fetchUserSettingsCompleteAction({
userSettings: normalizedUserSettings,
});
}),
```
This keeps all behaviour but removes the duplicated `startsWith("fe_dataset_table_")` / `replace(...)` branching.
### 2. Reuse the same config to simplify `updateUserSettings$`
Instead of keeping a separate `settingsToNest` array (which can easily diverge), build it from `SETTINGS_CONFIG`:
```ts
const SETTINGS_KEYS = SETTINGS_CONFIG.map((s) => s.key);
updateUserSettings$ = createEffect(() => {
return this.actions$.pipe(
ofType(fromActions.updateUserSettingsAction),
concatLatestFrom(() => [this.user$]),
takeWhile(([_, user]) => !!user),
switchMap(([{ property }, user]) => {
const propertyKeys = Object.keys(property) as SettingsKey[];
const newProperty: Record<string, any> = {};
const useExternalSettings = propertyKeys.some((k) =>
SETTINGS_KEYS.includes(k),
);
propertyKeys.forEach((key) => {
newProperty[key] = property[key];
});
const apiCall$ = useExternalSettings
? this.usersService.usersControllerPatchExternalSettingsV3(
user?.id,
JSON.stringify(newProperty) as any,
)
: this.usersService.usersControllerPatchSettingsV3(
user?.id,
JSON.stringify(newProperty) as any,
);
return apiCall$.pipe(
map((userSettings: UserSettings) =>
fromActions.updateUserSettingsCompleteAction({ userSettings }),
),
catchError(() => of(fromActions.updateUserSettingsFailedAction())),
);
}),
);
});
```
Now both fetching and updating use the same `SETTINGS_CONFIG`, removing duplication and reducing the chance of missing a key in one place.
### 3. Extract the conditions selection logic from `setConditions$`
The length-based choice between incoming and existing conditions can be hidden behind a pure helper, making the effect easier to scan:
```ts
function resolveConditions(
incoming: any[] | undefined,
existing: any[] | undefined,
) {
if (incoming && incoming.length > 0) {
return incoming;
}
return existing || [];
}
```
Then `setConditions$` becomes:
```ts
setConditions$ = createEffect(() => {
return this.actions$.pipe(
ofType(fromActions.fetchUserSettingsCompleteAction),
concatLatestFrom(() => this.store.select(selectConditions)),
mergeMap(([{ userSettings }, existingConditions]) => {
const incomingConditions =
(userSettings as any).externalSettings?.fe_dataset_table_conditions ||
[];
const conditions = resolveConditions(
incomingConditions,
existingConditions,
);
const actions = [];
conditions
.filter((condition) => condition.enabled)
.forEach((condition) => {
actions.push(
addScientificConditionAction({
condition: condition.condition,
}),
);
});
actions.push(
fromActions.updateConditionsConfigs({
conditionConfigs: conditions,
}),
);
return actions;
}),
);
});
```
This keeps the current behaviour (prefer incoming if any, otherwise fall back) but moves the decision logic into a small, testable function and makes the effect itself more declarative.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
4 tasks
Junjiequan
reviewed
Mar 26, 2026
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- SharedConditionComponent now hard-codes only two conditionSettingsKey values and repeats branching logic in ngOnInit, updateStore, updateConditionField, etc.; consider centralizing the mapping from conditionSettingsKey to selector/dispatch functions so adding new entities (or handling bad keys) is less fragile and avoids duplicated switch/if blocks.
- updateConditionsConfigs in the user reducer now only writes to settings.fe_dataset_table_conditions and no longer maintains the top-level state.conditions field, but UserState still declares conditions and some code/tests may still reference it; either keep that field in sync or remove it entirely to avoid subtle divergence.
- In fetchUserSettings$ the defaulting logic uses app config for dataset and proposal settings but falls back only to initialUserState.settings for sample, instrument, and file table settings; if there are corresponding default*ListSettings for those entities, it would be more consistent to use them as the primary source for defaults.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- SharedConditionComponent now hard-codes only two conditionSettingsKey values and repeats branching logic in ngOnInit, updateStore, updateConditionField, etc.; consider centralizing the mapping from conditionSettingsKey to selector/dispatch functions so adding new entities (or handling bad keys) is less fragile and avoids duplicated switch/if blocks.
- updateConditionsConfigs in the user reducer now only writes to settings.fe_dataset_table_conditions and no longer maintains the top-level state.conditions field, but UserState still declares conditions and some code/tests may still reference it; either keep that field in sync or remove it entirely to avoid subtle divergence.
- In fetchUserSettings$ the defaulting logic uses app config for dataset and proposal settings but falls back only to initialUserState.settings for sample, instrument, and file table settings; if there are corresponding default*ListSettings for those entities, it would be more consistent to use them as the primary source for defaults.
## Individual Comments
### Comment 1
<location path="src/app/shared/modules/shared-condition/shared-condition.component.ts" line_range="85" />
<code_context>
) {}
ngOnInit() {
+ switch (this.conditionSettingsKey) {
+ case "fe_dataset_table_conditions":
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing the conditionSettingsKey-specific selector and dispatch logic so that all condition updates flow through a single configuration and helper method.
You can centralize the `conditionSettingsKey` branching and remove the duplicate dispatch logic in `updateConditionField` by deriving the selector + update action once and always going through `updateStore`.
For example:
```ts
// class fields
conditionConfigs$;
private updateConditionConfigs: (conditionConfigs: ConditionConfig[]) => void;
```
Init both in one place:
```ts
ngOnInit() {
const configByKey = {
fe_dataset_table_conditions: {
selector: selectConditions,
updateAction: updateConditionsConfigs,
},
fe_sample_table_conditions: {
selector: selectSampleConditions,
updateAction: updateSampleConditionsConfigs,
},
} as const;
const config = configByKey[this.conditionSettingsKey];
if (!config) {
throw new Error("Invalid conditionSettingsKey");
}
this.conditionConfigs$ = this.store.select(config.selector);
this.updateConditionConfigs = (conditionConfigs: ConditionConfig[]) =>
this.store.dispatch(config.updateAction({ conditionConfigs }));
if (this.showConditions) {
this.buildMetadataMaps();
this.applyEnabledConditions();
}
}
```
Then `updateStore` no longer needs to re-switch on the key:
```ts
private updateStore(updatedConditions: ConditionConfig[]) {
this.updateConditionConfigs(updatedConditions);
this.store.dispatch(
updateUserSettingsAction({
property: { [this.conditionSettingsKey]: updatedConditions },
}),
);
}
```
And `updateConditionField` can delegate through `updateStore` instead of re-implementing the branching:
```ts
updateConditionField(index: number, updates: Partial<ScientificCondition>) {
this.subscriptions.push(
this.conditionConfigs$.pipe(take(1)).subscribe((conditions = []) => {
if (!conditions[index]) return;
const actualIndex = conditions.findIndex(
(c) => c.condition.lhs === conditions[index].condition.lhs,
);
if (actualIndex === -1) return;
const updatedConditions = [...conditions];
const conditionConfig = updatedConditions[actualIndex];
updatedConditions[actualIndex] = {
...conditionConfig,
condition: {
...conditionConfig.condition,
...updates,
human_name: this.humanNameMap[conditionConfig.condition.lhs],
},
};
this.updateStore(updatedConditions);
}),
);
}
```
This keeps all current behavior (including the new user settings key) but removes duplicated `if/else`/`switch` logic and consolidates the dataset vs sample mapping in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR updates selectors/reducers/effects, table components, condition filter handling and related tests with new externalSettings table keys for dataset, proposal, instrument and file entities.
New externalSettings structure looks like this:(SciCatProject/backend#2624)
{
externalSettings: {
fe_dataset_table_columns:{...},
fe_dataset_table_conditions:{...},
fe_dataset_table_filters:{...},
fe_proposal_table_columns:{...},
fe_proposal_table_filters:{...},
fe_sample_table_columns:{...},
fe_sample_table_conditions:{...},
fe_instrument_table_columns:{...},
fe_file_table_columns:{...},
}
}
Reason for the removal of selectColumnAction:
The selectColumnAction dispatch was removed because this flow is no longer functional. Enabling a scientific condition does not automatically add it to table columns, since scientific metadata keys are not part of the current column list. But we plan to introduce customizable column functionality in the future, and we will open a follow-up PR to remove the remaining legacy code related to this old flow.
Motivation
Background on use case, changes needed
Fixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Align frontend user settings, selectors, reducers, effects, and table/condition components with the new externalSettings structure and per-entity keys for columns, filters, and conditions.
Enhancements: