mirror of
https://github.com/4gray/iptvnator.git
synced 2026-10-08 17:06:15 -08:00
fix(settings): re-render OnPush sections when the form changes outside them
Review follow-ups (Greptile, Codex): - The settings sections read form values in their templates (selected theme and cover size, epgField.value, form().value.player), and the parent changes the form outside their events: Discard and backup import patch it, the store hydrates it, the EPG file picker sets a control after an await. Under OnPush the section kept the old selection or EPG status. Each section now marks itself on its form's events (markSectionForCheckOnFormEvents). - The value-only patch test no longer forces detectChanges(); with the fixture rendering on its own it fails without the marking, and so does a new test for a control set outside the EPG section. - The zoneless guard counts only changeDetection metadata outside comments, so a comment naming the strategy is not an Eager component. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
6d63285373
commit
a44d473cef
11 files changed
+144
-7
No files matched your search
@@ -70,9 +70,21 @@ function readEagerChecklist(): { open: string[]; done: string[] } {
|
||||
|
||||
const sources = readSources();
|
||||
|
||||
// Component metadata only: a comment or string that names the strategy is
|
||||
// not an Eager component.
|
||||
const eagerMetadata = /changeDetection\s*:\s*ChangeDetectionStrategy\.Eager\b/;
|
||||
|
||||
function withoutComments(text: string): string {
|
||||
return text.replace(/\/\*[\s\S]*?\*\//g, '').replace(/\/\/.*$/gm, '');
|
||||
}
|
||||
|
||||
function isEagerComponent(text: string): boolean {
|
||||
return eagerMetadata.test(withoutComments(text));
|
||||
}
|
||||
|
||||
test('the zoneless checklist lists exactly the components that are still Eager', () => {
|
||||
const eager = [...sources]
|
||||
.filter(([, text]) => text.includes('ChangeDetectionStrategy.Eager'))
|
||||
.filter(([, text]) => isEagerComponent(text))
|
||||
.map(([file]) => file)
|
||||
.sort();
|
||||
const { open } = readEagerChecklist();
|
||||
@@ -106,6 +118,23 @@ test('the guard skips test-only file names and keeps production ones', () => {
|
||||
}
|
||||
});
|
||||
|
||||
test('a comment that names the Eager strategy is not an Eager component', () => {
|
||||
assert.equal(
|
||||
isEagerComponent(
|
||||
'// was ChangeDetectionStrategy.Eager before C6\n' +
|
||||
'/* changeDetection: ChangeDetectionStrategy.Eager */\n' +
|
||||
'@Component({ changeDetection: ChangeDetectionStrategy.OnPush })'
|
||||
),
|
||||
false
|
||||
);
|
||||
assert.equal(
|
||||
isEagerComponent(
|
||||
'@Component({\n changeDetection: ChangeDetectionStrategy.Eager,\n})'
|
||||
),
|
||||
true
|
||||
);
|
||||
});
|
||||
|
||||
test('ticked checklist entries name files that exist', () => {
|
||||
for (const file of readEagerChecklist().done) {
|
||||
assert.ok(sources.has(file), `${file} is ticked but does not exist`);
|
||||
|
||||
@@ -19,6 +19,7 @@ import {
|
||||
ElectronBridgeAppUpdateStatus,
|
||||
} from '@iptvnator/shared/interfaces';
|
||||
import { UpdateChannelOption } from './settings.models';
|
||||
import { markSectionForCheckOnFormEvents } from './settings-section-form-render';
|
||||
|
||||
@Component({
|
||||
selector: 'app-settings-about-section',
|
||||
@@ -57,6 +58,13 @@ export class SettingsAboutSectionComponent {
|
||||
* setting. Absent in hosts that only render the version block.
|
||||
*/
|
||||
readonly form = input<FormGroup | null>(null);
|
||||
|
||||
constructor() {
|
||||
// Parent patches (Discard, backup import) change the form outside
|
||||
// this OnPush section's events.
|
||||
markSectionForCheckOnFormEvents(this.form);
|
||||
}
|
||||
|
||||
readonly updateChannelOptions = input<UpdateChannelOption[]>([]);
|
||||
|
||||
readonly buildCommitShort = computed(() => {
|
||||
|
||||
@@ -9,6 +9,7 @@ import { FormGroup, ReactiveFormsModule } from '@angular/forms';
|
||||
import { MatCheckboxModule } from '@angular/material/checkbox';
|
||||
import { MatIconModule } from '@angular/material/icon';
|
||||
import { TranslateModule } from '@ngx-translate/core';
|
||||
import { markSectionForCheckOnFormEvents } from './settings-section-form-render';
|
||||
|
||||
@Component({
|
||||
selector: 'app-settings-dashboard-section',
|
||||
@@ -26,4 +27,10 @@ import { TranslateModule } from '@ngx-translate/core';
|
||||
})
|
||||
export class SettingsDashboardSectionComponent {
|
||||
readonly form = input.required<FormGroup>();
|
||||
|
||||
constructor() {
|
||||
// Parent patches (Discard, backup import) change the form outside
|
||||
// this OnPush section's events.
|
||||
markSectionForCheckOnFormEvents(this.form);
|
||||
}
|
||||
}
|
||||
@@ -17,6 +17,7 @@ import { EpgViewMode } from '@iptvnator/shared/interfaces';
|
||||
import { EpgSourceStatusComponent } from '@iptvnator/ui/epg';
|
||||
import { TranslateModule } from '@ngx-translate/core';
|
||||
import { EpgViewModeOption } from './settings.models';
|
||||
import { markSectionForCheckOnFormEvents } from './settings-section-form-render';
|
||||
|
||||
@Component({
|
||||
selector: 'app-settings-epg-section',
|
||||
@@ -39,6 +40,13 @@ import { EpgViewModeOption } from './settings.models';
|
||||
})
|
||||
export class SettingsEpgSectionComponent {
|
||||
readonly form = input.required<FormGroup>();
|
||||
|
||||
constructor() {
|
||||
// Parent patches (Discard, backup import) change the form outside
|
||||
// this OnPush section's events.
|
||||
markSectionForCheckOnFormEvents(this.form);
|
||||
}
|
||||
|
||||
readonly epgUrl = input.required<FormArray>();
|
||||
readonly isClearingEpgData = input(false);
|
||||
readonly canBrowseFiles = input(false);
|
||||
|
||||
@@ -19,6 +19,7 @@ import {
|
||||
StartupWindowModeOption,
|
||||
ThemeOption,
|
||||
} from './settings.models';
|
||||
import { markSectionForCheckOnFormEvents } from './settings-section-form-render';
|
||||
|
||||
@Component({
|
||||
selector: 'app-settings-general-section',
|
||||
@@ -38,6 +39,13 @@ import {
|
||||
})
|
||||
export class SettingsGeneralSectionComponent {
|
||||
readonly form = input.required<FormGroup>();
|
||||
|
||||
constructor() {
|
||||
// Parent patches (Discard, backup import) change the form outside
|
||||
// this OnPush section's events.
|
||||
markSectionForCheckOnFormEvents(this.form);
|
||||
}
|
||||
|
||||
readonly languageEnum = input.required<typeof Language>();
|
||||
readonly themeOptions = input.required<ThemeOption[]>();
|
||||
readonly coverSizeOptions = input.required<CoverSizeOption[]>();
|
||||
|
||||
@@ -20,6 +20,7 @@ import {
|
||||
reportsPlaybackFailures,
|
||||
} from '@iptvnator/shared/interfaces';
|
||||
import { SettingsPlayerOption } from './settings.models';
|
||||
import { markSectionForCheckOnFormEvents } from './settings-section-form-render';
|
||||
|
||||
@Component({
|
||||
selector: 'app-settings-playback-section',
|
||||
@@ -52,6 +53,13 @@ export class SettingsPlaybackSectionComponent {
|
||||
].join('\n');
|
||||
|
||||
readonly form = input.required<FormGroup>();
|
||||
|
||||
constructor() {
|
||||
// Parent patches (Discard, backup import) change the form outside
|
||||
// this OnPush section's events.
|
||||
markSectionForCheckOnFormEvents(this.form);
|
||||
}
|
||||
|
||||
readonly players = input.required<SettingsPlayerOption[]>();
|
||||
readonly streamFormatEnum = input.required<typeof StreamFormat>();
|
||||
readonly isDesktop = input(false);
|
||||
|
||||
@@ -14,6 +14,7 @@ import { MatInputModule } from '@angular/material/input';
|
||||
import { MatTooltipModule } from '@angular/material/tooltip';
|
||||
import { TranslateModule } from '@ngx-translate/core';
|
||||
import { QRCodeComponent } from 'angularx-qrcode';
|
||||
import { markSectionForCheckOnFormEvents } from './settings-section-form-render';
|
||||
|
||||
@Component({
|
||||
selector: 'app-settings-remote-control-section',
|
||||
@@ -35,6 +36,13 @@ import { QRCodeComponent } from 'angularx-qrcode';
|
||||
})
|
||||
export class SettingsRemoteControlSectionComponent {
|
||||
readonly form = input.required<FormGroup>();
|
||||
|
||||
constructor() {
|
||||
// Parent patches (Discard, backup import) change the form outside
|
||||
// this OnPush section's events.
|
||||
markSectionForCheckOnFormEvents(this.form);
|
||||
}
|
||||
|
||||
readonly localIpAddresses = input.required<string[]>();
|
||||
readonly visibleQrCodeIp = input<string | null>(null);
|
||||
|
||||
|
||||
@@ -0,0 +1,30 @@
|
||||
import { ChangeDetectorRef, inject, type Signal } from '@angular/core';
|
||||
import { takeUntilDestroyed, toObservable } from '@angular/core/rxjs-interop';
|
||||
import type { AbstractControl } from '@angular/forms';
|
||||
import { EMPTY, switchMap } from 'rxjs';
|
||||
|
||||
/**
|
||||
* Marks an OnPush settings section for check on every event of its form.
|
||||
*
|
||||
* The sections read form values and states in their templates (selected
|
||||
* theme, `epgField.value`, `form().value.player`), which are not signals.
|
||||
* The parent changes the form outside the section's template events: Discard
|
||||
* and backup import patch it, the store hydrates it, and the EPG file picker
|
||||
* sets a control after an `await`. Without this the section keeps showing
|
||||
* the previous value until some unrelated event marks it. `events` covers
|
||||
* value, status, touched and pristine changes, including those of child
|
||||
* controls, which bubble up to the group.
|
||||
*
|
||||
* Call it from a field initializer or the constructor.
|
||||
*/
|
||||
export function markSectionForCheckOnFormEvents(
|
||||
form: Signal<AbstractControl | null>
|
||||
): void {
|
||||
const changeDetector = inject(ChangeDetectorRef);
|
||||
toObservable(form)
|
||||
.pipe(
|
||||
switchMap((control) => control?.events ?? EMPTY),
|
||||
takeUntilDestroyed()
|
||||
)
|
||||
.subscribe(() => changeDetector.markForCheck());
|
||||
}
|
||||
@@ -16,6 +16,7 @@ import { MatProgressSpinnerModule } from '@angular/material/progress-spinner';
|
||||
import { TranslateModule } from '@ngx-translate/core';
|
||||
import { TmdbApiService, TmdbCacheService } from '@iptvnator/services';
|
||||
import type { TmdbCacheStats } from '@iptvnator/shared/interfaces';
|
||||
import { markSectionForCheckOnFormEvents } from './settings-section-form-render';
|
||||
|
||||
type TmdbKeyTestState = 'idle' | 'testing' | 'success' | 'error';
|
||||
|
||||
@@ -91,6 +92,7 @@ export class SettingsTmdbSectionComponent {
|
||||
readonly isClearing = signal(false);
|
||||
|
||||
constructor() {
|
||||
markSectionForCheckOnFormEvents(this.form);
|
||||
// Sizing the cache is a full table scan, but this component only
|
||||
// exists while its section page is open, so loading on construction
|
||||
// preserves the old "wait until the user is actually looking"
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { FormArray, FormControl } from '@angular/forms';
|
||||
import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing';
|
||||
import { MatSnackBar } from '@angular/material/snack-bar';
|
||||
import { EpgRuntimeBridgeService } from '@iptvnator/epg/data-access';
|
||||
@@ -143,18 +144,21 @@ describe('SettingsComponent form', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// The sections are OnPush and a reset or backup import patches the
|
||||
// form outside their template events. A value-only patch changes no
|
||||
// form status signal, so the section must track the value itself.
|
||||
it('re-renders section selections after a value-only form patch', () => {
|
||||
// The sections are OnPush and a Discard or backup import patches the
|
||||
// form outside their template events, so the section must mark
|
||||
// itself on the form's events. The fixture renders on its own here:
|
||||
// a forced detectChanges() would hide a section that is not marked.
|
||||
it('re-renders section selections after a value-only form patch', async () => {
|
||||
const darkTheme = () =>
|
||||
(fixture.nativeElement as HTMLElement).querySelector(
|
||||
'[data-test-id="DARK_THEME"]'
|
||||
);
|
||||
fixture.autoDetectChanges();
|
||||
await fixture.whenStable();
|
||||
expect(darkTheme()?.getAttribute('aria-checked')).toBe('false');
|
||||
|
||||
component.settingsForm.patchValue({ theme: Theme.DarkTheme });
|
||||
fixture.detectChanges();
|
||||
await fixture.whenStable();
|
||||
|
||||
expect(darkTheme()?.getAttribute('aria-checked')).toBe('true');
|
||||
});
|
||||
@@ -201,6 +205,26 @@ describe('SettingsComponent form', () => {
|
||||
expect(settingsStore.updateSettings).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// The native file picker sets the EPG control after an await, with
|
||||
// no template event in the OnPush section; its status must follow.
|
||||
it('shows the source status after a control is set outside the section', async () => {
|
||||
setSettingsSection('epg');
|
||||
fixture.autoDetectChanges();
|
||||
const epgUrls = component.settingsForm.get('epgUrl') as FormArray;
|
||||
epgUrls.push(new FormControl(''));
|
||||
await fixture.whenStable();
|
||||
const status = () =>
|
||||
(fixture.nativeElement as HTMLElement).querySelector(
|
||||
'app-epg-source-status'
|
||||
);
|
||||
expect(status()).toBeNull();
|
||||
|
||||
epgUrls.at(epgUrls.length - 1).setValue('/tmp/guide.xml');
|
||||
await fixture.whenStable();
|
||||
|
||||
expect(status()).not.toBeNull();
|
||||
});
|
||||
|
||||
it('stages the EPG view mode without writing to the store until Save', () => {
|
||||
setSettingsSection('epg');
|
||||
fixture.detectChanges();
|
||||
|
||||
@@ -63,7 +63,12 @@ must be ticked here.
|
||||
two). Tick an entry by deleting `changeDetection: ChangeDetectionStrategy.Eager`
|
||||
(or setting OnPush) once its template state is signals, signal inputs or
|
||||
explicitly marked. The guard spec compares the unticked entries with the
|
||||
files that still contain `ChangeDetectionStrategy.Eager`.
|
||||
files whose component metadata still sets
|
||||
`changeDetection: ChangeDetectionStrategy.Eager` (comments do not count).
|
||||
The settings sections read form values in their templates and the parent
|
||||
patches the form outside their events (Discard, backup import, the EPG file
|
||||
picker), so each marks itself on the form's `events` through
|
||||
`markSectionForCheckOnFormEvents` (`apps/web/src/app/settings`).
|
||||
|
||||
### apps/web (15)
|
||||
|
||||
|
||||
Reference in new issue
Block a user