-
Notifications
You must be signed in to change notification settings - Fork 34
Swap 5087 scicat fe poc render frontend config with json fo #2143
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
base: master
Are you sure you want to change the base?
Swap 5087 scicat fe poc render frontend config with json fo #2143
Conversation
Bumps the types group with 2 updates: [@types/jasmine](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/jasmine) and [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node). Updates `@types/jasmine` from 5.1.12 to 5.1.13 - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/jasmine) Updates `@types/node` from 24.10.0 to 24.10.1 - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) --- updated-dependencies: - dependency-name: "@types/jasmine" dependency-version: 5.1.13 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: types - dependency-name: "@types/node" dependency-version: 24.10.1 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: types ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [cypress](https://github.com/cypress-io/cypress) from 15.6.0 to 15.7.0. - [Release notes](https://github.com/cypress-io/cypress/releases) - [Changelog](https://github.com/cypress-io/cypress/blob/develop/CHANGELOG.md) - [Commits](cypress-io/cypress@v15.6.0...v15.7.0) --- updated-dependencies: - dependency-name: cypress dependency-version: 15.7.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 3.14.1 to 3.14.2. - [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md) - [Commits](nodeca/js-yaml@3.14.1...3.14.2) --- updated-dependencies: - dependency-name: js-yaml dependency-version: 3.14.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
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.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
admin-config-edit.component.htmlthe top/bottom button bars are duplicated and all three actions (JSON preview, Export, Save) are wired tosave(), which makes it hard to understand intended behavior; consider removing the duplication and splitting these into separate, clearly named handlers (e.g.onPreview(),onExport(),onSave()) or adding TODOs if not yet implemented. - The
admin.reducercurrently logs every[Admin]action viaconsole.log, which will be noisy in production; consider removing this logging or guarding it behind an environment/feature flag. - Several new classes (e.g.
AdminDashboardComponent,ExpandGroupRendererComponent) inject or import dependencies that are not used (ChangeDetectorRef,ActivatedRoute,UsersService,MatDialog,JsonFormsAngularService,JsonFormsControl), so it would be good to remove these to keep the code and DI graphs lean.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `admin-config-edit.component.html` the top/bottom button bars are duplicated and all three actions (JSON preview, Export, Save) are wired to `save()`, which makes it hard to understand intended behavior; consider removing the duplication and splitting these into separate, clearly named handlers (e.g. `onPreview()`, `onExport()`, `onSave()`) or adding TODOs if not yet implemented.
- The `admin.reducer` currently logs every `[Admin]` action via `console.log`, which will be noisy in production; consider removing this logging or guarding it behind an environment/feature flag.
- Several new classes (e.g. `AdminDashboardComponent`, `ExpandGroupRendererComponent`) inject or import dependencies that are not used (`ChangeDetectorRef`, `ActivatedRoute`, `UsersService`, `MatDialog`, `JsonFormsAngularService`, `JsonFormsControl`), so it would be good to remove these to keep the code and DI graphs lean.
## Individual Comments
### Comment 1
<location> `src/app/app-routing/admin.guard.ts:39-42` </location>
<code_context>
): Observable<boolean> {
- return this.store.select(selectIsAdmin).pipe<boolean>(
- map((isAdmin: boolean) => {
+ return this.store.select(selectIsLoggedIn).pipe(
+ filter((isLoggedIn) => isLoggedIn),
+ switchMap(() => this.store.select(selectIsAdmin)),
+ map((isAdmin) => {
if (!isAdmin) {
this.router.navigate(["/401"], {
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard waits for a logged-in state but never resolves for anonymous users, which can leave navigation hanging.
Because the stream is filtered to only `true` values, `canActivate` never completes for anonymous users (or if `selectIsLoggedIn` never becomes `true`), so routing can hang instead of redirecting.
Handle the `false` case explicitly so the guard always resolves, e.g.:
- Take the first value from `selectIsLoggedIn` and branch on it, or
- Immediately navigate (e.g. to login/401/403) when `isLoggedIn === false` and return `false`.
</issue_to_address>
### Comment 2
<location> `src/app/_layout/app-header/app-header.component.html:83-85` </location>
<code_context>
</mat-menu>
<mat-menu #userMenu="matMenu">
+ <button mat-menu-item routerLink="/admin" data-cy="admin-button">
+ <mat-icon> build</mat-icon>
+ <span> Admin Settings <mat-chip class="beta-badge">Beta</mat-chip></span>
+ </button>
+
</code_context>
<issue_to_address>
**suggestion:** Admin menu entry is always shown, which leads non-admin users into a guarded route ending in a 401.
Since the route is guarded by `AdminGuard`, non-admins can still click this entry and get redirected to `/401`, which is a poor experience. Consider conditionally rendering or disabling this item for non-admin users (e.g. based on an `isAdmin` selector or feature flag), so only eligible users see it in the menu.
Suggested implementation:
```
<mat-menu #userMenu="matMenu">
<mat-menu #userMenu="matMenu">
<button
*ngIf="isAdmin$ | async"
mat-menu-item
routerLink="/admin"
data-cy="admin-button"
>
<mat-icon> build</mat-icon>
<span> Admin Settings <mat-chip class="beta-badge">Beta</mat-chip></span>
</button>
<button mat-menu-item routerLink="/user/" data-cy="setting-button">
<mat-icon> settings</mat-icon>
<span>Settings</span>
```
To fully implement this behavior, the component class (`app-header.component.ts`) will need:
1. An `isAdmin$` observable (e.g. `isAdmin$: Observable<boolean>;`) wired to your existing auth/permissions store or service (for example, via an NgRx selector like `this.isAdmin$ = this.store.select(selectIsAdmin)` or an auth service method).
2. Any required imports for the observable and state management/auth service being used.
If your codebase already exposes a different admin flag (e.g. `userIsAdmin$`, `canAccessAdmin$`), adjust the `*ngIf` expression to use that instead.
</issue_to_address>
### Comment 3
<location> `src/app/admin/admin-config-edit/admin-config-edit.component.html:4-10` </location>
<code_context>
+<div style="display: flex; justify-content: space-between; margin-bottom: 10px">
+ <!-- LEFT -->
+ <div>
+ <button mat-button color="primary" (click)="save()">Json Preview</button>
+ </div>
+
+ <!-- RIGHT -->
+ <div>
+ <button mat-button color="primary" (click)="save()">Export</button>
+ <button mat-button color="primary" (click)="save()">Save</button>
+ </div>
+</div>
</code_context>
<issue_to_address>
**issue (bug_risk):** All three actions (Json Preview, Export, Save) are wired to the same `save()` handler, which is confusing and likely incorrect.
Using the same `save()` handler for all three buttons prevents different logic for preview, export, and save, and is probably unintended. Please either give each button a dedicated handler (e.g. `openPreview()`, `export()`, `save()`) or pass a mode into `save` (e.g. `(click)="save('preview')"`) and branch on that inside the method.
</issue_to_address>
### Comment 4
<location> `src/app/admin/admin-config-edit/admin-config-edit.component.ts:45-54` </location>
<code_context>
+ currentData: any = {};
</code_context>
<issue_to_address>
**issue (bug_risk):** Saving before any change sends an empty object instead of the loaded configuration.
Because `currentData` is initialized as `{}` and only updated in `onChange`, calling `save()` before any `dataChange` will persist `{}` and wipe the existing config. Initialize `currentData` from the loaded config (e.g. first emission from `data$`/`config$`), or have `save()` fall back to the latest value from `data$` when `currentData` is still the initial `{}`.
</issue_to_address>
### Comment 5
<location> `src/app/state-management/reducers/admin.reducer.ts:28-29` </location>
<code_context>
+);
+
+export const adminReducer = (state: AdminState | undefined, action: Action) => {
+ if (action.type.indexOf("[Admin]") !== -1) {
+ console.log("Action came in! " + action.type);
+ }
+ return reducer(state, action);
</code_context>
<issue_to_address>
**suggestion (performance):** Reducer logs every admin action to the console, which is noisy and not ideal for production.
If this is for debugging, consider using NgRx store devtools, guarding the `console.log` with an environment/debug flag, or moving the logging into a meta-reducer so it doesn’t run unconditionally in production.
Suggested implementation:
```typescript
export const adminReducer = (state: AdminState | undefined, action: Action) => {
if (!environment.production && action.type.includes('[Admin]')) {
// Debug logging for admin actions; disabled in production builds
// eslint-disable-next-line no-console
console.log('Admin action dispatched:', action.type, action);
}
return reducer(state, action);
};
```
To make this compile, also add an environment import at the top of the same file:
- Add:
`import { environment } from '../../../environments/environment';`
Place it alongside the other imports in `admin.reducer.ts`, respecting the existing import ordering conventions (typically third-party imports first, then application imports).
</issue_to_address>
### Comment 6
<location> `src/app/admin/admin-dashboard/admin-dashboard.component.ts:32` </location>
<code_context>
+ paths: "exact",
+ };
+
+ fetchDataActions: { [tab: string]: { action: any; loaded: boolean } } = {
+ [TAB.configuration]: { action: "", loaded: false },
+ [TAB.usersList]: { action: "", loaded: false },
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the premature tab-loading abstraction (TAB enum usage, fetchDataActions, and fetchDataForTab) to keep the component focused on its current, simple behavior.
You can simplify this component by removing the unused tab-loading abstraction until it’s actually needed. Right now `TAB`, `fetchDataActions`, and `fetchDataForTab` introduce indirection without behavior.
### 1. Remove `fetchDataActions` and `fetchDataForTab` (for now)
They don’t do anything meaningful yet and just add cognitive load.
```ts
export class AdminDashboardComponent implements OnInit {
showError = false;
navLinks: {
location: string;
label: string;
icon: string;
enabled: boolean;
}[] = [];
routerLinkActiveOptions: IsActiveMatchOptions = {
matrixParams: "ignored",
queryParams: "ignored",
fragment: "ignored",
paths: "exact",
};
constructor(
public appConfigService: AppConfigService,
private cdRef: ChangeDetectorRef,
private route: ActivatedRoute,
private userService: UsersService,
public dialog: MatDialog,
) {}
ngOnInit(): void {
this.navLinks = [
{
location: "./configuration",
label: TAB.configuration,
icon: "menu",
enabled: true,
},
{
location: "./usersList",
label: TAB.usersList,
icon: "data_object",
enabled: true,
},
];
}
onTabSelected(tab: string) {
// For now, if specific tabs need loading, handle them directly here:
if (tab === TAB.configuration) {
// load configuration data
} else if (tab === TAB.usersList) {
// load users list data
}
}
}
```
When you actually need generic tab-loading, you can reintroduce a minimal and concrete abstraction informed by real use-cases (e.g., only once multiple tabs share the same loading pattern).
### 2. Optionally simplify label handling
If `TAB` is only used for labels and not for any logic, you can also drop the enum and use literals directly to reduce indirection:
```ts
ngOnInit(): void {
this.navLinks = [
{
location: "./configuration",
label: "Configuration",
icon: "menu",
enabled: true,
},
{
location: "./usersList",
label: "Users List",
icon: "data_object",
enabled: true,
},
];
}
```
You can keep the enum if you expect these labels to be reused elsewhere soon, but otherwise literals keep the component straightforward.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return this.store.select(selectIsLoggedIn).pipe( | ||
| filter((isLoggedIn) => isLoggedIn), | ||
| switchMap(() => this.store.select(selectIsAdmin)), | ||
| map((isAdmin) => { |
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.
issue (bug_risk): Guard waits for a logged-in state but never resolves for anonymous users, which can leave navigation hanging.
Because the stream is filtered to only true values, canActivate never completes for anonymous users (or if selectIsLoggedIn never becomes true), so routing can hang instead of redirecting.
Handle the false case explicitly so the guard always resolves, e.g.:
- Take the first value from
selectIsLoggedInand branch on it, or - Immediately navigate (e.g. to login/401/403) when
isLoggedIn === falseand returnfalse.
| <button mat-menu-item routerLink="/admin" data-cy="admin-button"> | ||
| <mat-icon> build</mat-icon> | ||
| <span> Admin Settings <mat-chip class="beta-badge">Beta</mat-chip></span> |
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.
suggestion: Admin menu entry is always shown, which leads non-admin users into a guarded route ending in a 401.
Since the route is guarded by AdminGuard, non-admins can still click this entry and get redirected to /401, which is a poor experience. Consider conditionally rendering or disabling this item for non-admin users (e.g. based on an isAdmin selector or feature flag), so only eligible users see it in the menu.
Suggested implementation:
<mat-menu #userMenu="matMenu">
<mat-menu #userMenu="matMenu">
<button
*ngIf="isAdmin$ | async"
mat-menu-item
routerLink="/admin"
data-cy="admin-button"
>
<mat-icon> build</mat-icon>
<span> Admin Settings <mat-chip class="beta-badge">Beta</mat-chip></span>
</button>
<button mat-menu-item routerLink="/user/" data-cy="setting-button">
<mat-icon> settings</mat-icon>
<span>Settings</span>
To fully implement this behavior, the component class (app-header.component.ts) will need:
- An
isAdmin$observable (e.g.isAdmin$: Observable<boolean>;) wired to your existing auth/permissions store or service (for example, via an NgRx selector likethis.isAdmin$ = this.store.select(selectIsAdmin)or an auth service method). - Any required imports for the observable and state management/auth service being used.
If your codebase already exposes a different admin flag (e.g.userIsAdmin$,canAccessAdmin$), adjust the*ngIfexpression to use that instead.
| <button mat-button color="primary" (click)="save()">Json Preview</button> | ||
| </div> | ||
|
|
||
| <!-- RIGHT --> | ||
| <div> | ||
| <button mat-button color="primary" (click)="save()">Export</button> | ||
| <button mat-button color="primary" (click)="save()">Save</button> |
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.
issue (bug_risk): All three actions (Json Preview, Export, Save) are wired to the same save() handler, which is confusing and likely incorrect.
Using the same save() handler for all three buttons prevents different logic for preview, export, and save, and is probably unintended. Please either give each button a dedicated handler (e.g. openPreview(), export(), save()) or pass a mode into save (e.g. (click)="save('preview')") and branch on that inside the method.
| currentData: any = {}; | ||
| schema: any = schema.schema || {}; | ||
| uiSchema: any = schema.uiSchema || {}; | ||
| renderers = [ | ||
| ...angularMaterialRenderers, | ||
| { | ||
| tester: accordionArrayLayoutRendererTester, | ||
| renderer: AccordionArrayLayoutRendererComponent, | ||
| }, | ||
| { |
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.
issue (bug_risk): Saving before any change sends an empty object instead of the loaded configuration.
Because currentData is initialized as {} and only updated in onChange, calling save() before any dataChange will persist {} and wipe the existing config. Initialize currentData from the loaded config (e.g. first emission from data$/config$), or have save() fall back to the latest value from data$ when currentData is still the initial {}.
| if (action.type.indexOf("[Admin]") !== -1) { | ||
| console.log("Action came in! " + action.type); |
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.
suggestion (performance): Reducer logs every admin action to the console, which is noisy and not ideal for production.
If this is for debugging, consider using NgRx store devtools, guarding the console.log with an environment/debug flag, or moving the logging into a meta-reducer so it doesn’t run unconditionally in production.
Suggested implementation:
export const adminReducer = (state: AdminState | undefined, action: Action) => {
if (!environment.production && action.type.includes('[Admin]')) {
// Debug logging for admin actions; disabled in production builds
// eslint-disable-next-line no-console
console.log('Admin action dispatched:', action.type, action);
}
return reducer(state, action);
};To make this compile, also add an environment import at the top of the same file:
- Add:
import { environment } from '../../../environments/environment';
Place it alongside the other imports in admin.reducer.ts, respecting the existing import ordering conventions (typically third-party imports first, then application imports).
Description
Short description of the pull request
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
Introduce an admin area for managing runtime frontend configuration via JSONForms, wired through NgRx and backend runtime-config APIs, and expose it through the header for admin users.
New Features:
Bug Fixes:
Enhancements:
Build: