feat: configurable about page#2396
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new AppConfigInterface properties (helpEnabled, infoEnabled, infoHtmlFile, infoHtmlContent) are non-optional, which may break existing configs; consider marking them optional or assigning defaults in initializeApp to keep backward compatibility.
- AboutComponent uses bypassSecurityTrustHtml on config-provided HTML; if this content can be edited by admins or comes from outside the trusted codebase, consider introducing basic sanitization/validation or documenting the trust assumptions to avoid XSS risks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new AppConfigInterface properties (helpEnabled, infoEnabled, infoHtmlFile, infoHtmlContent) are non-optional, which may break existing configs; consider marking them optional or assigning defaults in initializeApp to keep backward compatibility.
- AboutComponent uses bypassSecurityTrustHtml on config-provided HTML; if this content can be edited by admins or comes from outside the trusted codebase, consider introducing basic sanitization/validation or documenting the trust assumptions to avoid XSS risks.
## Individual Comments
### Comment 1
<location path="src/app/app-config.service.ts" line_range="175-178" />
<code_context>
statusBannerMessage?: string;
statusBannerCode?: "INFO" | "WARN";
autoApplyFilters?: boolean;
+ helpEnabled: boolean;
+ infoEnabled: boolean;
+ infoHtmlFile: string;
+ infoHtmlContent: string;
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider making new config fields optional or providing defaults for backward compatibility.
These new properties (`helpEnabled`, `infoEnabled`, `infoHtmlFile`, `infoHtmlContent`) are required on `AppConfigInterface`, but existing `config.json` files won’t define them. This makes the type inconsistent with real configs and can lead to confusing type narrowing or extra boilerplate. Consider marking them as optional (`?:`) or initializing them to sane defaults so the interface accurately matches what’s actually present at runtime.
Suggested implementation:
```typescript
statusBannerMessage?: string;
statusBannerCode?: "INFO" | "WARN";
autoApplyFilters?: boolean;
helpEnabled?: boolean;
infoEnabled?: boolean;
infoHtmlFile?: string;
infoHtmlContent?: string;
}
```
```typescript
if (config.helpEnabled == null) {
config.helpEnabled = false;
}
if (config.infoEnabled == null) {
config.infoEnabled = false;
}
if (!config.infoHtmlFile) {
// Default info HTML file path for backward compatibility; adjust as needed.
config.infoHtmlFile = 'assets/info.html';
}
if (!config.infoHtmlContent && config.infoHtmlFile) {
try {
config.infoHtmlContent = await firstValueFrom(
this.http.get(config.infoHtmlFile, { responseType: 'text' }).pipe(timeout(2000)),
);
} catch (err) {
```
Depending on how `helpEnabled` and `infoEnabled` are used elsewhere in the app, you may also want to:
1. Replace direct property access (`config.helpEnabled`) with null-safe defaults (e.g. `config.helpEnabled ?? false`) where appropriate.
2. Update any tests that assume these fields are required so they either set explicit values or assert against the new defaults.
</issue_to_address>
### Comment 2
<location path="src/app/app-config.service.ts" line_range="288-291" />
<code_context>
config.datasetPageSizeOptions = [5, 10, 25, 100];
}
+ if (!config.infoHtmlContent) {
+ try {
+ config.infoHtmlContent = await firstValueFrom(
+ this.http.get(config.infoHtmlFile,{ responseType: 'text' }).pipe(timeout(2000)),
+ );
+ } catch (err) {
</code_context>
<issue_to_address>
**issue:** Guard against missing `infoHtmlFile` and consider honoring `infoEnabled`.
If `infoHtmlFile` is missing in `config.json`, this will call `http.get(undefined)` and hit `'/undefined'` before the catch assigns the default. To avoid noisy 404s and better use `infoEnabled`, only call `http.get` when `infoHtmlFile` is truthy (and/or `infoEnabled` is true); otherwise assign the default HTML directly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| helpEnabled: boolean; | ||
| infoEnabled: boolean; | ||
| infoHtmlFile: string; | ||
| infoHtmlContent: string; |
There was a problem hiding this comment.
suggestion (bug_risk): Consider making new config fields optional or providing defaults for backward compatibility.
These new properties (helpEnabled, infoEnabled, infoHtmlFile, infoHtmlContent) are required on AppConfigInterface, but existing config.json files won’t define them. This makes the type inconsistent with real configs and can lead to confusing type narrowing or extra boilerplate. Consider marking them as optional (?:) or initializing them to sane defaults so the interface accurately matches what’s actually present at runtime.
Suggested implementation:
statusBannerMessage?: string;
statusBannerCode?: "INFO" | "WARN";
autoApplyFilters?: boolean;
helpEnabled?: boolean;
infoEnabled?: boolean;
infoHtmlFile?: string;
infoHtmlContent?: string;
} if (config.helpEnabled == null) {
config.helpEnabled = false;
}
if (config.infoEnabled == null) {
config.infoEnabled = false;
}
if (!config.infoHtmlFile) {
// Default info HTML file path for backward compatibility; adjust as needed.
config.infoHtmlFile = 'assets/info.html';
}
if (!config.infoHtmlContent && config.infoHtmlFile) {
try {
config.infoHtmlContent = await firstValueFrom(
this.http.get(config.infoHtmlFile, { responseType: 'text' }).pipe(timeout(2000)),
);
} catch (err) {Depending on how helpEnabled and infoEnabled are used elsewhere in the app, you may also want to:
- Replace direct property access (
config.helpEnabled) with null-safe defaults (e.g.config.helpEnabled ?? false) where appropriate. - Update any tests that assume these fields are required so they either set explicit values or assert against the new defaults.
| if (!config.infoHtmlContent) { | ||
| try { | ||
| config.infoHtmlContent = await firstValueFrom( | ||
| this.http.get(config.infoHtmlFile,{ responseType: 'text' }).pipe(timeout(2000)), |
There was a problem hiding this comment.
issue: Guard against missing infoHtmlFile and consider honoring infoEnabled.
If infoHtmlFile is missing in config.json, this will call http.get(undefined) and hit '/undefined' before the catch assigns the default. To avoid noisy 404s and better use infoEnabled, only call http.get when infoHtmlFile is truthy (and/or infoEnabled is true); otherwise assign the default HTML directly.
Description
This PR adds the feature to configure the about page with a customized html text, which ca nbe inserted in configuration or in a dedicated file. If the file is selected, it can be mounted in directly in the container
Motivation
the current about page contains outdated text and hard coded text.
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
AI contribution
This PR contains code created with mistral.ai (chat.mistral.ai, version n/a)
Summary by Sourcery
Make the About page content configurable via HTML supplied from application configuration or an external file, replacing hard-coded facility-specific text.
New Features:
Enhancements: