test(notes): harden #8434 fix per multi-review

Apply verified findings from a multi-agent review of the resize-close fix:

- Guard the navigation listener on `dialogRef.getState() === OPEN`. A
  breakpoint-crossing resize can emit more than one popstate, and
  MatDialogRef.close() does not check its own state — a second call would
  re-run the exit animation and schedule a duplicate close timeout. The
  guard makes the second+ fires no-ops (the saved result still resolves
  once, so this was churn, not data loss).
- Add unit tests asserting the navigation-close actually PERSISTS the
  edit (the point of #8434), both via `changed` when alive and via the
  direct dispatch after a breakpoint switch destroys the host (#8432),
  plus a test that a second popstate while closing does not re-close.
- e2e: hover the notes area before clicking the opacity-hidden
  fullscreen control, matching the real user flow.
- Tighten the duplicated explanatory comment.

No behavior change to the fix itself. 77 inline-markdown unit tests and
the note-resize e2e pass.
This commit is contained in:
Johannes Millan 2026-06-17 13:43:20 +02:00
parent 5966ba5f43
commit 19e2cbd63a
3 changed files with 70 additions and 17 deletions

View file

@ -32,8 +32,11 @@ test.describe('Detail panel fullscreen note - resize', () => {
await expect(detailPanel).toBeVisible();
// Open the fullscreen markdown editor from the notes inline-markdown.
await detailPanel
.locator('inline-markdown button')
// The controls are opacity:0 until the notes area is hovered.
const noteMarkdown = detailPanel.locator('inline-markdown').first();
await noteMarkdown.hover();
await noteMarkdown
.locator('button')
.filter({ has: page.locator('mat-icon', { hasText: 'fullscreen' }) })
.first()
.click();

View file

@ -1,5 +1,5 @@
import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing';
import { MatDialog } from '@angular/material/dialog';
import { MatDialog, MatDialogState } from '@angular/material/dialog';
import { MarkdownModule } from 'ngx-markdown';
import { NoopAnimationsModule } from '@angular/platform-browser/animations';
import { InlineMarkdownComponent } from './inline-markdown.component';
@ -1867,15 +1867,18 @@ describe('InlineMarkdownComponent', () => {
describe('fullscreen editor save-and-close on navigation (#8434)', () => {
let afterClosed$: Subject<unknown>;
let store: MockStore;
let closeSpy: jasmine.Spy;
let unsubscribeSpy: jasmine.Spy;
let locationCb: ((value: PopStateEvent) => void) | undefined;
let dialogState: MatDialogState;
beforeEach(() => {
afterClosed$ = new Subject<unknown>();
closeSpy = jasmine.createSpy('close');
unsubscribeSpy = jasmine.createSpy('unsubscribe');
locationCb = undefined;
dialogState = MatDialogState.OPEN;
// Capture the Location listener so a "navigation" can be simulated.
const location = TestBed.inject(Location);
@ -1887,11 +1890,16 @@ describe('InlineMarkdownComponent', () => {
mockMatDialog.open.and.returnValue({
afterClosed: () => afterClosed$.asObservable(),
componentInstance: { close: closeSpy },
getState: () => dialogState,
} as never);
store = TestBed.inject(MockStore);
spyOn(store, 'dispatch');
fixture.componentRef.setInput('taskId', 'task-1');
fixture.detectChanges();
});
const navigate = (): void => locationCb!({} as PopStateEvent);
// The default closeOnNavigation disposes the overlay with no result on a
// navigation, dropping the edit (#8434); we must opt out so we can close it
// through the save path instead.
@ -1909,11 +1917,52 @@ describe('InlineMarkdownComponent', () => {
component.openFullScreen();
expect(closeSpy).not.toHaveBeenCalled();
locationCb!({} as PopStateEvent);
navigate();
expect(closeSpy).toHaveBeenCalledTimes(1);
});
// A breakpoint-crossing resize can emit more than one popstate; the second
// must not re-trigger close() and re-run the dialog's exit animation.
it('does not close again once the dialog is already closing', () => {
component.openFullScreen();
navigate();
dialogState = MatDialogState.CLOSING;
navigate();
expect(closeSpy).toHaveBeenCalledTimes(1);
});
// The point of #8434: a navigation-close must PERSIST the edit, not just
// close. When still alive the note routes out via `changed`.
it('persists the edit via `changed` when a navigation closes the dialog', () => {
spyOn(component.changed, 'emit');
component.openFullScreen();
navigate();
// The dialog resolves through its save path with the typed content.
afterClosed$.next('typed before resize');
expect(component.changed.emit).toHaveBeenCalledWith('typed before resize');
});
// The production scenario: the breakpoint switch destroys this host while
// the editor is open, so the save must land via the direct dispatch (#8432).
it('persists directly when a navigation closes the dialog after host destroy', () => {
component.openFullScreen();
component.ngOnDestroy();
navigate();
afterClosed$.next('typed before resize');
expect(store.dispatch).toHaveBeenCalledWith(
TaskSharedActions.updateTask({
task: { id: 'task-1', changes: { notes: 'typed before resize' } },
}),
);
});
it('stops listening for navigations once the dialog has closed', () => {
component.openFullScreen();

View file

@ -17,7 +17,7 @@ import {
} from '@angular/core';
import { FormsModule } from '@angular/forms';
import { MatIconButton } from '@angular/material/button';
import { MatDialog } from '@angular/material/dialog';
import { MatDialog, MatDialogState } from '@angular/material/dialog';
import { MatIcon } from '@angular/material/icon';
import { MatMenu, MatMenuItem, MatMenuTrigger } from '@angular/material/menu';
import { MatTooltip } from '@angular/material/tooltip';
@ -382,18 +382,19 @@ export class InlineMarkdownComponent implements OnInit, OnDestroy {
},
});
// Reinstate close-on-navigation as a save-and-close. closeOnNavigation
// (above) is the overlay's `disposeOnNavigation`, which on a Location
// change (popstate/hashchange) DISPOSES the overlay with no result —
// dropping the edit. We listen to the same Location signal but close
// through the dialog's save path, so an Android back-button press or the
// involuntary navigation a breakpoint-crossing resize triggers persists
// the note instead of losing it. Not tied to takeUntilDestroyed so it
// still fires after a breakpoint switch destroys this host mid-edit; torn
// down in the afterClosed handler below.
const locationSub = this._location.subscribe(() =>
dialogRef.componentInstance?.close(),
);
// Reinstate close-on-navigation as a save-and-close: listen to the same
// Location signal closeOnNavigation uses, but route through the dialog's
// save path so an Android back-button press or the breakpoint-crossing
// resize above persists the note instead of dropping it. Not tied to
// takeUntilDestroyed so it still fires after a breakpoint switch destroys
// this host mid-edit; torn down in the afterClosed handler below.
const locationSub = this._location.subscribe(() => {
// A breakpoint-crossing resize can emit more than one popstate; only act
// while the dialog is still open so we don't re-run its exit animation.
if (dialogRef.getState() === MatDialogState.OPEN) {
dialogRef.componentInstance?.close();
}
});
// Intentionally NOT torn down with takeUntilDestroyed: this MUST still fire
// after the component is destroyed — see the `_isDestroyed` branch below.