WEB-714: Add submit button to user settings#3142
Open
DeathGun44 wants to merge 1 commit intoopenMF:devfrom
Open
WEB-714: Add submit button to user settings#3142DeathGun44 wants to merge 1 commit intoopenMF:devfrom
DeathGun44 wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Templates & styles src/app/settings/settings.component.html, src/app/settings/settings.component.scss |
Adds a .submit-actions container and a Submit button bound to submit(); button enabled only when hasChanges is true; right-aligned styling (display:flex; justify-content:flex-end; margin-top:16px). |
Settings component logic src/app/settings/settings.component.ts |
Replaces per-field subscriptions with trackChanges() that merges valueChanges; introduces hasChanges flag and initialValues, hasFormChanged() helper, and submit() to save settings via SettingsService; injects AlertService and TranslateService; keeps destroy$ cleanup and updates lifecycle signature. |
Translations src/assets/translations/en-US.json |
Adds translation entry for the settings success message (e.g., text.Settings saved successfully → Settings saved successfully.) used by the success alert. |
Sequence Diagram(s)
sequenceDiagram
participant User as User
participant Comp as SettingsComponent
participant Service as SettingsService
participant Trans as TranslateService
participant Alert as AlertService
rect rgba(210,230,255,0.5)
User->>Comp: Click Submit
end
rect rgba(200,255,200,0.5)
Comp->>Service: saveAllSettings(payload)
Service-->>Comp: success
end
rect rgba(255,240,200,0.5)
Comp->>Trans: translate("text.Settings saved successfully")
Trans-->>Comp: localizedMessage
Comp->>Alert: success(localizedMessage)
Alert-->>Comp: ack
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'WEB-714: Add submit button to user settings' directly and clearly summarizes the main change—adding a submit button to the settings component. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Tip
Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
IOhacker
requested changes
Feb 14, 2026
| this.hasChanges = false; | ||
| this.alertService.alert({ | ||
| type: 'Settings Update', | ||
| message: 'Settings saved successfully.' |
Contributor
There was a problem hiding this comment.
Is there a way to translate this message?
Contributor
Author
There was a problem hiding this comment.
Hi @IOhacker
I looked into this and found that none of the ~30 alertService.alert() calls across the codebase are currently translated — they're all hardcoded English.
I think a better approach would be a dedicated ticket to standardize alert i18n across the entire codebase. Should I open one? For this PR I can use the translation key with TranslateService.instant() so it's translation-ready, and the actual translated strings can be added when the broader i18n effort lands.
Alternatively, I can add the key to en-US.json now and leave the other 12 language files for the i18n ticket.
Contributor
Author
|
@IOhacker could you please take a look at my message when you have a moment! If it looks good i will proceed. |
Contributor
|
@DeathGun44 we will open a new PR, please complete this one. |
DeathGun44
force-pushed
the
WEB-714/enable-submit-to-user-settings
branch
from
7c311c8 to
d2d57e9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/settings/settings.component.ts`:
- Around line 127-137: The trackChanges subscription only sets hasChanges to
true and never resets it; capture an initial snapshot of the monitored fields
(language, dateFormat, datetimeFormat, decimalsToDisplay) when the form/data is
loaded, then on each change compare current values to that snapshot inside
trackChanges (use the same controls: this.language.value, this.dateFormat.value,
this.datetimeFormat.value, this.decimalsToDisplay.value) and set this.hasChanges
= true only if any value differs, otherwise set this.hasChanges = false so
reverting values clears the flag.
- Around line 141-144: The FormControl.value properties (language, dateFormat,
datetimeFormat, decimalsToDisplay) are typed as string | null and being passed
to non-nullable setters (settingsService.setLanguage, setDateFormat,
setDatetimeFormat, setDecimalToDisplay); update calls to coalesce nulls to safe
defaults (e.g., use nullish coalescing to fall back to the service getters like
settingsService.getLanguage(), getDateFormat(), getDatetimeFormat(),
getDecimalToDisplay()) or alternatively explicitly type the FormControls at
declaration (e.g., FormControl<string> or
FormControl<{name:string;code:string}>) so the .value is non-nullable before
calling those setters.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/settings/settings.component.htmlsrc/app/settings/settings.component.scsssrc/app/settings/settings.component.tssrc/assets/translations/en-US.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/settings/settings.component.html
DeathGun44
force-pushed
the
WEB-714/enable-submit-to-user-settings
branch
from
d2d57e9 to
d73de6d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/settings/settings.component.ts (1)
111-120: Tighten form state typing (any→ explicit model types).
FormControl<any>on line 111 andinitialValues.language: anyon line 119 weaken compile-time guarantees. TheSettingsService.setLanguage()signature shows the expected type is{ name: string; code: string }, so align the form control and state model to this explicit type instead of usingany.♻️ Proposed refactor
+type LanguageOption = { name: string; code: string }; + - language = new FormControl<any>(''); + language = new FormControl<LanguageOption | null>(null); ... - private initialValues: { language: any; dateFormat: string; datetimeFormat: string; decimals: string }; + private initialValues: { language: LanguageOption | null; dateFormat: string; datetimeFormat: string; decimals: string };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/settings/settings.component.ts` around lines 111 - 120, The language FormControl and initialValues.language are typed too loosely as any; change the language FormControl generic to the explicit shape { name: string; code: string } and update the initialValues type signature so initialValues.language: { name: string; code: string } (keep other properties as-is), then fix any downstream usages (e.g., places that call SettingsService.setLanguage or read initialValues.language) to match the stronger type; update imports/types if needed so the new object type is reused consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/settings/settings.component.ts`:
- Around line 111-120: The language FormControl and initialValues.language are
typed too loosely as any; change the language FormControl generic to the
explicit shape { name: string; code: string } and update the initialValues type
signature so initialValues.language: { name: string; code: string } (keep other
properties as-is), then fix any downstream usages (e.g., places that call
SettingsService.setLanguage or read initialValues.language) to match the
stronger type; update imports/types if needed so the new object type is reused
consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/settings/settings.component.htmlsrc/app/settings/settings.component.scsssrc/app/settings/settings.component.tssrc/assets/translations/en-US.json
DeathGun44
force-pushed
the
WEB-714/enable-submit-to-user-settings
branch
from
d73de6d to
1ede337
Compare
Contributor
Author
|
@IOhacker Done! |
Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
DeathGun44
force-pushed
the
WEB-714/enable-submit-to-user-settings
branch
from
1ede337 to
5a97323
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/settings/settings.component.ts`:
- Around line 133-153: The form dirty-tracking and submit flow currently only
include dateFormat, datetimeFormat, and decimalsToDisplay (see trackChanges,
hasFormChanged, submit) while Language changes are persisted immediately via
LanguageSelectorComponent.setLanguage() calling settingsService.setLanguage(),
causing inconsistent behavior; fix by adding language into the same
initialValues/state, include its FormControl in merge(...) inside trackChanges,
add its comparison in hasFormChanged (compare (this.language.value ?? '') to
this.initialValues.language), and update submit() to call
settingsService.setLanguage(this.language.value ?? this.initialValues.language)
so language follows the unified save flow (or conversely, remove immediate
settingsService.setLanguage call from LanguageSelectorComponent.setLanguage and
delegate to parent submit if you prefer scoping the PR to exclude language).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45427ba6-37e7-4db7-9b4f-e490f26f819b
📒 Files selected for processing (4)
src/app/settings/settings.component.htmlsrc/app/settings/settings.component.scsssrc/app/settings/settings.component.tssrc/assets/translations/en-US.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/assets/translations/en-US.json
Comment on lines
+133
to
+153
| trackChanges(): void { | ||
| merge(this.dateFormat.valueChanges, this.datetimeFormat.valueChanges, this.decimalsToDisplay.valueChanges) | ||
| .pipe(takeUntil(this.destroy$)) | ||
| .subscribe(() => { | ||
| this.hasChanges = this.hasFormChanged(); | ||
| }); | ||
| } | ||
|
|
||
| private hasFormChanged(): boolean { | ||
| return ( | ||
| (this.dateFormat.value ?? '') !== this.initialValues.dateFormat || | ||
| (this.datetimeFormat.value ?? '') !== this.initialValues.datetimeFormat || | ||
| (this.decimalsToDisplay.value ?? '') !== this.initialValues.decimals | ||
| ); | ||
| } | ||
|
|
||
| submit(): void { | ||
| this.settingsService.setDateFormat(this.dateFormat.value ?? this.initialValues.dateFormat); | ||
| this.settingsService.setDatetimeFormat(this.datetimeFormat.value ?? this.initialValues.datetimeFormat); | ||
| this.settingsService.setDecimalToDisplay(this.decimalsToDisplay.value ?? this.initialValues.decimals); | ||
| this.initialValues = { |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
#!/bin/bash
set -euo pipefail
echo "== Settings component: language wiring =="
rg -n -C3 "setLanguage|language|initialValues|trackChanges|submit\(" src/app/settings/settings.component.ts
echo
echo "== Language selector implementation =="
fd 'language-selector.component.ts$' src/app -x sh -c '
echo "--- $1 ---"
rg -n -C3 "SettingsService|setLanguage|valueChanges|FormControl|localStorage|Output|EventEmitter" "$1"
' shRepository: openMF/web-app
Repository: openMF/web-app
Exit code: 0
stdout:
== Settings component: language wiring ==
24-} from '@angular/material/expansion';
25-import { FileUploadComponent } from '../shared/file-upload/file-upload.component';
26-import { ThemePickerComponent } from '../shared/theme-picker/theme-picker.component';
27:import { LanguageSelectorComponent } from '../shared/language-selector/language-selector.component';
28-import { STANDALONE_SHARED_IMPORTS } from 'app/standalone-shared.module';
29-
30-/**
--
112- /** Decimals to Display Setting */
113- decimalsToDisplay = new FormControl('');
114-
115: private initialValues: {
116- dateFormat: string;
117- datetimeFormat: string;
118- decimals: string;
119- };
120-
121- ngOnInit() {
122: this.initialValues = {
123- dateFormat: this.settingsService.dateFormat,
124- datetimeFormat: this.settingsService.datetimeFormat,
125- decimals: this.settingsService.decimals
126- };
127: this.dateFormat.patchValue(this.initialValues.dateFormat, { emitEvent: false });
128: this.datetimeFormat.patchValue(this.initialValues.datetimeFormat, { emitEvent: false });
129: this.decimalsToDisplay.patchValue(this.initialValues.decimals, { emitEvent: false });
130: this.trackChanges();
131- }
132-
133: trackChanges(): void {
134- merge(this.dateFormat.valueChanges, this.datetimeFormat.valueChanges, this.decimalsToDisplay.valueChanges)
135- .pipe(takeUntil(this.destroy$))
136- .subscribe(() => {
--
140-
141- private hasFormChanged(): boolean {
142- return (
143: (this.dateFormat.value ?? '') !== this.initialValues.dateFormat ||
144: (this.datetimeFormat.value ?? '') !== this.initialValues.datetimeFormat ||
145: (this.decimalsToDisplay.value ?? '') !== this.initialValues.decimals
146- );
147- }
148-
149: submit(): void {
150: this.settingsService.setDateFormat(this.dateFormat.value ?? this.initialValues.dateFormat);
151: this.settingsService.setDatetimeFormat(this.datetimeFormat.value ?? this.initialValues.datetimeFormat);
152: this.settingsService.setDecimalToDisplay(this.decimalsToDisplay.value ?? this.initialValues.decimals);
153: this.initialValues = {
154- dateFormat: this.dateFormat.value ?? '',
155- datetimeFormat: this.datetimeFormat.value ?? '',
156- decimals: this.decimalsToDisplay.value ?? ''
== Language selector implementation ==
--- src/app/shared/language-selector/language-selector.component.ts ---
8-
9-/** Angular Imports */
10-import { Component, inject, Input } from '@angular/core';
11:import { UntypedFormControl, ReactiveFormsModule } from '@angular/forms';
12-
13-/** Custom Services */
14-import { TranslateService } from '@ngx-translate/core';
15:import { SettingsService } from 'app/settings/settings.service';
16-import { STANDALONE_SHARED_IMPORTS } from 'app/standalone-shared.module';
17-
18-/**
--
31-})
32-export class LanguageSelectorComponent {
33- private translateService = inject(TranslateService);
34: private settingsService = inject(SettingsService);
35-
36- /** Appearance of the form field (fill, outline). Defaults to 'fill'. */
37- `@Input`() appearance: 'fill' | 'outline' = 'fill';
--
40- `@Input`() showLabel: boolean = true;
41-
42- /** Language selector form control. */
43: languageSelector = new UntypedFormControl();
44-
45- /**
46- * Sets the language of the application in the selector on initial setup.
--
54- * Sets a new language to be used by the application.
55- * `@param` {string} language New language.
56- */
57: setLanguage() {
58- this.translateService.use(this.languageSelector.value);
59: this.settingsService.setLanguage({ name: '', code: this.languageSelector.value.substring(0, 2) });
60- }
61-
62- /**
Language is not included in the new submit-based save flow.
trackChanges() and submit() handle only dateFormat, datetimeFormat, and decimalsToDisplay. The Language setting is persisted through a separate mechanism in LanguageSelectorComponent.setLanguage(), which directly calls settingsService.setLanguage() without going through the parent component's dirty-check or submit flow.
This creates inconsistent behavior: Language changes persist immediately while other settings now require explicit submission. Either wire Language into the same dirty-check + submit path, or explicitly scope this PR to exclude Language changes from the unified save behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/settings/settings.component.ts` around lines 133 - 153, The form
dirty-tracking and submit flow currently only include dateFormat,
datetimeFormat, and decimalsToDisplay (see trackChanges, hasFormChanged, submit)
while Language changes are persisted immediately via
LanguageSelectorComponent.setLanguage() calling settingsService.setLanguage(),
causing inconsistent behavior; fix by adding language into the same
initialValues/state, include its FormControl in merge(...) inside trackChanges,
add its comparison in hasFormChanged (compare (this.language.value ?? '') to
this.initialValues.language), and update submit() to call
settingsService.setLanguage(this.language.value ?? this.initialValues.language)
so language follows the unified save flow (or conversely, remove immediate
settingsService.setLanguage call from LanguageSelectorComponent.setLanguage and
delegate to parent submit if you prefer scoping the PR to exclude language).
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
2 participants
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
Replaced the silent auto-save behavior in User Settings with an explicit Submit button. Previously, the Main Configuration fields (Language, Date Format, Decimals to Display) were auto-saved on every dropdown change via
valueChangessubscriptions with no user feedback. This change adds:AlertService("Settings saved successfully.")takeUntilto prevent memory leaksThe submit button covers all previously working fields: Language, Date Format, and Decimals to Display.
Screenshots
Checklist
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Style